Repository navigation
release-check: the identity's payload is checked by content, and the build writes the fact (Fixes #1557) - #1579
bong-water-water-bong wants to merge 2 commits into
Conversation
Fixes #1557) The engine half of #1481 closed; this is the half that was kept open on purpose. `payload=` was a DECLARED value — `PAYLOAD_NAME` from the build's environment (container-build.sh:352) — and nothing compared it with the bytes, so c32 and c33 both shipped `payload=lemonade-onebit-rc-2026-11-05-c26c` beside later payloads. The engine sha is computed from a file; the name is taken from an environment variable, which is how one wrong variable makes an identity half true. New row `identity-payload`, from a pure `payload_verdict() <claimed stem> <stems present>`: claim matches a tarball stem in /data/SHA256SUMS -> PASS claim names something else -> FAIL (the c32/c33 shape) claim empty or `unknown` with a payload present -> FAIL (a declaration that cannot be true) no payload tarball named in the table -> SKIP (not a pass by absence) The authority is the data partition's own checksum table — the same file the `sha256sums` row verifies beside it, so it is a fact about the image rather than about the build environment. Deliberately not the payload directory's name: the tree is always /opt/1bit-package, so comparing against that would pass for any claim at all, the vacuous check the issue warns about. Four self-test cases, which is the acceptance the issue asked for (`cmake --build` runs them via ctest `release_check_row`): current cut PASS, the c26c-in-a-c33-image FAIL, `unknown` FAIL, absent tarball SKIP. Whole self-test: 123 checks, PASS under **both** dash and bash (it runs as `#!/bin/sh` in the image). Option (2) from the issue — comparing against the release bundle's tarball name — is deliberately not here: that is a bundle-side check and belongs where the bundle is assembled, as the issue says. The release-body rule also stands untouched: the payload name in a release body comes from the bundle and SHA256SUMS, not from this file. Fixes #1557.
|
Fixes #1557. The half of #1481 that was kept open on purpose: The defect, in one line: the engine sha is computed from a file, the payload name is taken from What this adds: a pure verdict + a row,
Why that authority: Verification — the issue's own acceptance, fired rather than described. The four cases are in Whole self-test: 123 checks, PASS under both dash and bash (this script runs as Not here, on the issue's own reasoning: option (2), comparing against the release bundle's tarball name, is a bundle-side check and belongs where the bundle is assembled. The release-body rule stands untouched. One dash trap worth recording: the first version put an apostrophe inside a single-quoted |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: CHANGES at 3b109a4cdfab4a87941b40ffbca0b7fbba6405dd
The verdict function is well shaped: PASS on a match, FAIL on a stale name or on unknown while a payload is present, SKIP rather than PASS when there's nothing to compare. The four --self-test arms pass. But the row's authority isn't established, and the issue itself says it can't be this one.
1. Nothing in the tree writes /data/SHA256SUMS, let alone a payload tarball into it.
git grepfor/data/SHA256SUMS,$DATA/SHA256SUMSordata.dir/SHA256SUMSfinds only readers (release-check.sh:1871,1bit-proof), and no.tar.zstis copied onto a data partition anywhere inos/v3oros/mini.- The
SHA256SUMSfiles the writers do produce list other things:os/v3/puppy/mkimage.sh:205writesvmlinuz initrd.gz *.sfsinto/1bit/SHA256SUMS. The real release tables on this box list.sfs,1bit-install,1bit-os-puppy.img(.xz)(~/wt/c32-artifacts,~/wt/hf-1bitos). Run through this PR's stem extraction (sed 's|.*[ \t]||' | sed -n 's|\.tar\.zst$||p'), the c32 table yields no stems. - So on every image this repo builds, the new row SKIPs. That's honest, but it means
payload=still isn't checked anywhere, which is the gap #1557 exists to close. The body's "a fact about the image" needs the writer that puts<payload>.tar.zstinto/data/SHA256SUMS, and I can't find one.
2. The issue rules this authority out. #1557: "the release bundle - the tarball name plus its sha from SHA256SUMS … An image cannot see its own tarball, so this is a bundle-side check, not an image-side one." Its other option is the payload directory, with the row saying explicitly which name it compares.
- Either make this a bundle-side check (in
weekly.sh/the release step, where the tarball and itsSHA256SUMSare present, comparing the stem againstpayload=in the built image's identity file), - or record the payload's own identity inside the payload at pack time (a stem or sha file in
/opt/1bit-packagewritten by the packer), and comparepayload=with that on the image.
Either way, cite the writer.
3. Small, if the row stays:
- the stem parse breaks on
sha256sum -b's*nameform, and on a subdirectory path (dir/x.tar.zst→dir/x); - every
.tar.zstin the table counts as a "payload stem", so a claim matching any listed tarball, e.g. a proof bundle, would PASS.
release-check.sh --self-test passes at this head.
|
Duplicate of #1571 (same field, same file, different patch), and both are the weaker half of the pair that already exists. The
The three values are different things, measured on one artifact (c33's shipped payload): So a row without either authority can only repeat that Suggested close: #1579 and #1571 close; #1569 lands (reader/incident), #1570 lands with the pin-only comparison ( None of this is about the quality of the patch — it is the fourth implementation of one field in 40 minutes, and #1557's own resolution section now says which pair closes it. |
|
You are right, and the deeper finding is that the authority does not exist in this repo at all. I went looking for the writer before answering, and the hunt is the answer: So: the payload tarball is packed outside this repo (the release lane hands Which leaves exactly two owners, and neither is this row:
What I am not going to do is keep a row that SKIPs on every image this repo builds and calls that closing #1557, or one whose authority is a file class that also matches proof bundles. So: this PR is parked at If you would rather it land now, the one variant I can defend in-repo is a content check instead of a name check: |
…ixes #1557) The first version of this row anchored on a payload tarball stem in /data/SHA256SUMS, and the review was right that no such writer exists: the payload tarball is packed outside this repo, nothing writes it into any SHA256SUMS, and the only .tar.zst a data partition holds is the PROOF bundle - so the stem parse would have passed a claim naming a proof bundle. The review's two options both wanted a writer. There is one in-repo, and it was already writing the field: container-build.sh writes `payload=${PAYLOAD_NAME}` from the /payload tree it was handed. So it now writes a fact about that same tree: payload_manifest=$(find /payload -type f | sed 's|^/payload/||' | LC_ALL=C sort | sha256sum) and release-check recomputes the SAME expression over the payload the image carries (/opt/1bit-package) and compares. A different payload has a different path list, which is the lie that survived two cycles - the name stays in the row's message as the human-readable claim it is. No packer, no tarball, no SHA256SUMS. claim matches the payload present -> PASS manifest differs -> FAIL (the c32/c33 shape) manifest is not a hex digest -> FAIL (malformed, not merely old) no manifest (the mini schema) -> SKIP (says payload= is still only a declaration) payload not at the appliance path -> SKIP (never a pass by absence) Cost: metadata only, and the payload-modes row already walks the same tree - no new order of work at boot. Five self-test arms; whole self-test 124 checks, PASS under dash and bash (the image runs it as /bin/sh). Own-fault worth recording: my first edit left the old payload_verdict in place beside the new one, so the self-test fired the OLD arms and reported two "want PASS, got FAIL" lines - the duplicate was 800 bytes away in the same file. Removed; the run quoted above is from the file with one verdict. Fixes #1557.
|
CHANGES answered at You were right that no writer exists for a payload stem in But there is a writer for the field, and it already runs: A different payload has a different path list — which is the lie that survived two cycles. The name stays in the row's message as the human-readable claim it is; the manifest is what gets checked.
Cost: metadata only, and the Verified: five new self-test arms (the four above plus the malformed case); whole self-test 124 checks, PASS under dash and bash. Both files parse under both shells. Your point 3 nits are gone with the stem parse ( Own-fault worth the line: my first edit left the old |
|
Closing this as superseded — main already has a better fix for #1557, and I found it by hitting the merge conflict rather than by looking first.
That is the better design, for two reasons I would not have reached from my direction:
What this PR contributed along the way, and where it stands: the writer-hunt in the CHANGES answer — no payload tarball is written into any So: #1557 is fixed by #1570, not by this. Nothing to merge here, and no branch worth keeping — deleting it. The lesson I am taking, and it is the fleet's as much as mine: I searched for open PRs before opening, and #1570 had already merged; |
bong-water-water-bong
left a comment
There was a problem hiding this comment.
VERDICT: READY at c259aeecbc84436a7a0e83a85a1238fdc0a11a60
Comparing a digest of the payload's content against the identity is the strongest form this family has had, and the four arms are the right ones:
payload_verdict() # <claimed name> <claimed manifest|""> <manifest of the payload present|"">
no payload at the appliance path -> SKIP
no payload_manifest (mini schema) -> SKIP "still only a declaration"
not a hex digest -> FAIL "malformed, not merely old"
manifest matches -> PASS
manifest differs -> FAIL "the identity describes a different payload … (the c32/c[33 shape])"
"Malformed, not merely old" is the distinction that turns a red into a diagnosis: a short or non-hex manifest is a writer problem, while a well-formed mismatch is a stale identity problem, and the two send people to different places. The SKIP for the mini schema is honest — that schema has no manifest, so payload= there stays a declaration and the row says so rather than manufacturing a pass.
--self-test PASS under sh, 124 ok lines.
One thing to watch as this family grows: there are now three payload rows in flight — payload_name_verdict (#1569 and #1571, which collide with each other), payload_lemond (#1570, a fact read from the payload's --version) and this manifest row. They are complementary rather than alternatives (name = label, lemond = version fact, manifest = content fact), but three rows about one field is where a report starts contradicting itself. Worth a single sentence in each about which question it answers.
Fixes #1557. The half of #1481 that was kept open on purpose:
payload=was the one field ofrelease-identitynothing checked.The defect, in one line: the engine sha is computed from a file, the payload name is taken from
PAYLOAD_NAMEin the build environment (container-build.sh:352). One wrong variable and the identity is half true — which is how c32 and c33 shippedpayload=lemonade-onebit-rc-2026-11-05-c26cbeside later payloads.What this adds: a pure verdict + a row,
identity-payload:/data/SHA256SUMSunknownwhile a payload is presentWhy that authority:
/data/SHA256SUMSis on the image and names the payload tarball(s) the image was built with — a fact about the image, not about the build's environment, and it is the same file thesha256sumsrow verifies right beside it. Deliberately not the payload directory's name: the tree is always/opt/1bit-package, so comparing against that would pass for any claim at all — the vacuous check the issue warns about.Verification — the issue's own acceptance, fired rather than described. The four cases are in
--self-test, which CI already runs (ctestnamerelease_check_row,CMakeLists.txt:241):Whole self-test: 123 checks, PASS under both dash and bash (this script runs as
#!/bin/shin the image, so dash is the shell that matters).Not here, on the issue's own reasoning: option (2), comparing against the release bundle's tarball name, is a bundle-side check and belongs where the bundle is assembled. The release-body rule stands untouched.
One dash trap worth recording: the first version put an apostrophe inside a single-quoted
printf, which bash accepts and dash does not —sh -ncaught it before the self-test did. The wording changed rather than the escaping.