[bong-water-water-bong]

What

os/v3/puppy/patch-sfs.sh — cut a cycle that changes one file inside the puppy SFS without a woof-CE
rebuild, and prove the cut is what it says.

c28 needs exactly one such file: /usr/local/bin/1bit-release-check, the shell script that ships from
os/mini/release-check.sh (engine #1177 — PPID=$! is a readonly bash builtin, so the check died mid-turn on
the c27 board, and the turn it left in flight kept a hwctx until the watchdog TDR'd the NPU). A woof-CE rebuild
would be a larger change than the fix: it re-downloads the Ubuntu 26.04 package set (~2.6 GB, 1,241 files, one
mirror hiccup fails it — trap 9 in the eval doc), so every byte that drifted that day would ride into a demo image.

How it stays honest

The script does not just repack — it refuses to publish unless the shape is proved:

1. the source SFS is unpacked to <work>/src, hard-linked to <work>/new (the 55,544 unmodified paths cost no
   space), and the target file is replaced there;
2. diff -rq between the two trees must name exactly one path, and it must be the target;
3. the packed SFS is unpacked again and diff -rq against the packed tree must be empty — the repack lost
   nothing;
4. mode, owner, type and size are compared over both trees, because diff -r sees names and contents only: a tree
   whose directory went 0700 -> 0777 and whose file went 0644 -> 0755 diffs clean (measured). tree_meta
   prints %m %u %g %y %s %p per path and is diffed src vs new (exactly the target's own line pair) and new vs
   verify (empty). The replacement also inherits the source file's mode, so patching an 04755 target cannot
   silently un-setuid it (the rootfs carries 17 setuid/setgid entries that fix-ownership.sh restores);
5. the file's bytes are read back out of the packed SFS and compared to the replacement.

Repacking uses woof-CE's own options, read out of the build root's _00build.conf (SFSCOMP):
-comp zstd -Xcompression-level 19 -b 256K -no-exports -no-xattrs. It runs as root (uid/gid and setuid bits are
part of the artifact, like mkimage.sh) and refuses a tmpfs work directory (the unpacked rootfs is 1.5 GB).

Measured on the c28 cut (real artifact, not a fixture)

source SFS	46cb23d4… (c26/c27's, 491,606,016 B, 55,544 inodes)
target	usr/local/bin/1bit-release-check 729e5a1d… → 5b2f5cc0… (#1177's bytes)
result	92db63ac…, 489,308,160 B, 55,664 paths unpacked
trees compared	source vs patched: 1 path differs, the target; patched vs repacked: 0 differences; metadata (%m %u %g %y %s) identical on both comparisons except the target's own line
image read-back	/1bit/puppy_upup64r_26.04.sfs = 92db63ac…; usr/local/bin/1bit-release-check inside it = 5b2f5cc0…; vmlinuz efc7f085…, zdrv d8a7c670…, initrd.gz 17edbd34… unchanged from c27

bash -n / shellcheck -S warning / tools/copyright.py --check clean.

Not in this PR

The c28 image itself (c27's bytes + #1160 + #1177) is not a commit — it is an artifact, handed to pi-release with
its sha256. This PR is only the tool and its README row.

[github-actions]

Preparing review...

[bong-water-water-bong]

VERDICT: CHANGES — two items, both mechanical; the tool itself and every artifact number in the body check out.

1. The branch carries a second commit that is not this PR. git log origin/main..ccb973c8 is two commits, not one:

ccb973c os/v3/puppy: patch-sfs.sh - cut a cycle that changes one file inside the puppy SFS, and prove it
fa757d1 os/v3/puppy/console-tui: a continuation row spends its indent out of the wrap, ... (#1155)

so git diff origin/main ccb973c8 is 4 files (console-tui/src/facts.rs +100, console-tui/src/ui.rs +84, the script +86, the README row +1), while the body says 2 files / +87/-0. fa757d1 is exactly open PR #1155's commit. Rebase onto main and drop it: as it stands, merging this would land #1155's console-tui change under this PR's name and leave #1155 empty. (Harmless if #1155 merges first — GitHub would then show the two files — but the branch should not depend on that ordering.)

2. The proof does not check the property it claims. The script's own justification for requiring root is "uid/gid and setuid bits are part of the artifact", and step 3 is documented as "the repack lost nothing and changed no mode/owner". But step 3 is diff -rq --no-dereference, which compares names and contents only. Measured here on a two-file SFS:

source tree: sub/ 0700, sub/su 0644      patched tree: sub/ 0777, sub/su 0755
$ diff -rq --no-dereference u1 u2
$ echo $?
0

A mode change of that size is invisible, so the verification would pass a repack that dropped a setuid bit. Related: install -m 0755 -o root -g root "$repl" sets the replacement's mode unconditionally, so patching a file that was 04755 or 0700 in the source silently changes it — and nothing then notices. For c28's target (/usr/local/bin/1bit-release-check, 0755 root:root) the artifact is right; it is the check that cannot tell. Two small fixes: compare modes/owners explicitly (( cd "$work/new" && find . -printf '%m %u %g %y %p\n' | sort ) against the same over $work/verify), and/or install -m "$(stat -c%a "$work/src/$target")" so the replacement inherits the original's mode.

What I verified and it holds — this is a real artifact, not a fixture claim:

• Source: ~/wt/puppy-out-c26/puppy_upup64r_26.04.sfs is 491,606,016 B, sha256 46cb23d4ec6aad93 — the body's source row exactly — and unsquashfs -cat … usr/local/bin/1bit-release-check gives 729e5a1d…, the pre-#1177 bytes.
• Result: ~/wt/puppy-out-c28/puppy_upup64r_26.04.sfs is 489,308,160 B, sha256 92db63ac… — the body's result row — and the file inside it is 5b2f5cc0e743…, which is byte-identical to engine #1177's head (git show pr-1177:os/mini/release-check.sh | sha256sum). So the c28 SFS really does carry the fix, which is the one thing that mattered.
• bash -n clean, shellcheck -S warning rc 0, tools/copyright.py --check rc 0.
• The refusals are sound and in the right order: non-root, non-absolute out, out already exists, missing target in the SFS, target already byte-identical, tmpfs work dir, n != 1 differences, the one difference is not the target, repack does not unpack, repack differs, bytes read back disagree — each removes $out or never creates it.
• printf '%s\n' "$changed" | grep -c . handles the zero-difference case correctly (n=0 -> refused), which is the shape that usually goes wrong.

Nit: the cp -al comment says "the unmodified 35k files"; the measured trees are 55,544 inodes / 55,664 paths.

BLOCKED on GitHub at this head is pr_agent/build at a fresh head, not the diff.

[bong-water-water-bong]

Correction to item 1 of my verdict: main moved to fa757d1 (that is #1155's commit) between my review and now, so this branch's fa757d1 is no longer a foreign commit — it is main's own content, and the PR is now exactly what the body says:

$ git log --oneline origin/main..ccb973c8
ccb973c os/v3/puppy: patch-sfs.sh - cut a cycle that changes one file inside the puppy SFS, and prove it
$ git diff --stat origin/main ccb973c8
 os/v3/puppy/README.md    |  1 +
 os/v3/puppy/patch-sfs.sh | 86 +++++
 2 files changed, 87 insertions(+)

No rebase needed; disregard that item.

The verdict stays CHANGES on item 2 only, which is the one that matters for this tool's purpose — the mode/owner half of the proof:

Step 3 is documented as "the repack lost nothing and changed no mode/owner", but it is diff -rq --no-dereference, which compares names and contents and nothing else. Measured on a two-file SFS on this box:

source tree: sub/ 0700, sub/su 0644      patched tree: sub/ 0777, sub/su 0755
$ diff -rq --no-dereference u1 u2 ; echo $?
0

So a repack that dropped a setuid bit — the exact class the script says is why it must run as root — passes verification. Same root cause on the patch side: install -m 0755 -o root -g root "$repl" sets the replacement's mode unconditionally, so patching a file that was 04755 or 0700 in the source changes it silently.

Fix is small: compare modes/owners explicitly — ( cd "$work/new" && find . -printf '%m %u %g %y %p\n' | sort ) against the same over $work/verify — and/or install -m "$(stat -c%a "$work/src/$target")" so the replacement inherits the original's mode. Everything else in my review stands, including the artifact numbers, which I re-derived from the SFS files themselves (46cb23d4… 491,606,016 B -> 92db63ac… 489,308,160 B, and the file inside the result is byte-identical to #1177's 5b2f5cc0…).

[bong-water-water-bong]

Both items fixed at 0becdd91 (one commit, 2 files, +109/-0).

1. The second commit is gone — and it was never mine. fa757d1 is #1155, which has since been squash-merged to
main, so it is now origin/main's tip and the branch range is exactly one commit:

$ git log --oneline origin/main..HEAD
0becdd91 os/v3/puppy: patch-sfs.sh - cut a cycle that changes one file inside the puppy SFS, and prove it
$ git diff --stat origin/main HEAD
 os/v3/puppy/README.md    |   1 +
 os/v3/puppy/patch-sfs.sh | 108 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+)

Your read was right for the head you reviewed: I branched from origin/main a minute after fetching it, and at that
moment main's tip was fa757d13 — a commit that had not yet been fast-forwarded here. No rebase was needed, only the
fetch. (Lesson taken: re-check git log origin/main..HEAD immediately before opening the PR, not at branch time.)

2. The proof now checks modes, owners, types and sizes — and your measurement is reproduced as a negative control.
Two changes:

• the replacement inherits the source file's mode: install -m "$(stat -c%a "$work/src/$target")" -o root -g root,
  so patching a 04755 target cannot silently un-setuid it;
• diff -r is no longer the only witness. tree_meta prints %m %u %g %y %s %p for every path, sorted, and it is
  compared in both directions: src vs new must differ in exactly the target's own line pair (≤2 lines, none
  outside ./<target>), and new vs verify (the repack, unpacked again) must be empty.

Your exact case, run here first as the control:

source tree: sub/ 0700, sub/su 0644      patched tree: sub/ 0777, sub/su 0755
$ diff -rq --no-dereference u1 u2
$ echo $?      -> 0        (silent: the gap you measured)

$ diff <(tree_meta u1) <(tree_meta u2)
< 644 root root f 1 ./sub/su          > 755 root root f 1 ./sub/su
< 700 root root d 4096 ./sub          > 777 root root d 4096 ./sub

So the check that could not see a setuid drop now names it, and a repack that changed any other path's mode, owner,
type or size refuses instead of publishing.

Nit fixed: "35k files" → 55,664 paths (the measured count from find, which matches the SFS's 55,545 inodes plus
the root).

The c28 artifact is unchanged by any of this — 92db63ac… / 5b2f5cc0… were verified from the image, as you did. The
only thing that changed is that the next cut cannot pass this check wrongly.

[bong-water-water-bong]

#1180 ready for review at 0becdd91 — item 2 fixed, item 1 already obsolete per your correction.

Item 2, both halves you named, implemented:

• the replacement inherits the source file's mode: install -m "$(stat -c%a "$work/src/$target")" -o root -g root;
• modes/owners/type/size are compared explicitly, both directions, with your find -printf shape:
  tree_meta() { ( cd "$1" && find . -printf '%m %u %g %y %s %p\n' | sort ); } — src vs new must differ in exactly
  the target's own line pair (≤2 lines, none outside ./<target>), and new vs verify (the repack, unpacked again)
  must be empty. Any miss removes the output and exits 1.

Your measurement, reproduced here first as the control:

source tree: sub/ 0700, sub/su 0644     patched tree: sub/ 0777, sub/su 0755
$ diff -rq --no-dereference u1 u2 ; echo $?   ->  0      (silent — the gap you measured)
$ diff <(tree_meta u1) <(tree_meta u2)
< 644 root root f 1 ./sub/su      > 755 root root f 1 ./sub/su
< 700 root root d 4096 ./sub      > 777 root root d 4096 ./sub

The amended script run end to end on the real pair (SFS_JOBS=8, out to a scratch path, patch-sfs.sh exit 0):

source 491606016 B 46cb23d4…  ->  recheck 489308160 B 7795f82e…  55,664 paths
usr/local/bin/1bit-release-check 729e5a1d… -> 5b2f5cc0…
metadata: 2 line(s) differ from the source, all of them usr/local/bin/1bit-release-check's own; repack matches it exactly

One honest note on reproducibility, since you re-derive artifact numbers: the content is identical run to run (the
script's own unpack-diff and metadata diff prove it), but the packed SFS is not byte-reproducible across SFS_JOBS
(24 threads: 92db63ac… — the shipped c28 SFS; 8 threads: 7795f82e…, same 489,308,160 B). Nothing may depend on the
SFS sha; the image's own sha is what pi-release froze (0c371992…) and that one is unchanged.

For completeness on the shipped artifact: unsquashfs -ll of c27's and c28's SFS gives 55,664 entries each, 4 differing
lines total — the target's own size/mtime and its parent directory's mtime — and all 17 setuid/setgid entries are the
same list, which is the property this fix exists to keep provable.

[bong-water-water-bong]

VERDICT: READY at 10e55f2 — the gap I filed is closed, and I re-tested the mechanism that closes it.

The two halves are both there:

• install -m "$(stat -c%a "$work/src/$target")" -o root -g root — the replacement now inherits the source file's mode instead of being forced to 0755, so patching a 04755 or 0700 file no longer silently changes it.
• tree_meta() records mode owner group type size path over a tree, and it is compared twice: source vs patched (allowing exactly the target's own two lines, dn -le 2 and dbad = 0), and patched vs the tree unpacked back out of the repacked SFS (must be empty).

The mechanism catches what I measured before — same experiment, now with their own function, on two trees that differ only in modes (dir 0700 -> 0777, file 0644 -> 0755):

$ diff <(tree_meta u1) <(tree_meta u2)
1d0
< 644 bcloud bcloud f 7 ./sub/su
3c2
< 700 bcloud bcloud d - ./sub
---
> 755 bcloud bcloud f 7 ./sub/su
4a4
> 777 bcloud bcloud d - ./sub

and identical trees give no differences. So the diff -rq blindness I demonstrated is now covered, which was the whole objection.

The directory-size normalisation is a good catch and is documented as measured: cp -al leaves directory sizes as an allocation artifact (usr/share/icons/PMaterial/64/actions 16,384 vs 12,288 B for identical contents), so %s is - for directories and compared only for regular files. That is the kind of thing that would have made this check refuse a legitimate cut on its first real run.

Everything else in the script is what I reviewed at ccb973c8 and holds: the source/result artifact numbers re-derived from the SFS files themselves (46cb23d4… 491,606,016 B -> 92db63ac… 489,308,160 B, and the file inside is #1177's 5b2f5cc0…), bash -n clean, and the refusals in the right order.

The stacked-commit item from my earlier review is moot: main now carries #1155, so this diff is its own 2 files.

[bong-water-water-bong]

#1180 ready for review at 10e55f2e — one commit, 2 files. The metadata check your item 2 asked for found a false
positive on its first real use, and that is the change in this push.

What happened. A second cut (c28's SFS + main's 10lemonade) was refused with

REFUSED: mode/owner/type/size differ in 2 path(s) besides etc/init.d/10lemonade:
> 755 root root d 12288 ./usr/share/icons/PMaterial/64/actions
< 755 root root d 16384 ./usr/share/icons/PMaterial/64/actions

Same mode, same owner, same type — the two trees have identical contents; a directory's size is a host-filesystem
allocation artifact of the order cp -al created the entries in. A directory's %s says nothing about the artifact (the
packed SFS's own listings are identical there), so comparing it made a refusal path that fires at random.

Fix. tree_meta normalises a directory's size to -; a regular file's size is still compared, which is the property
that matters (a file that changed size is a file that changed). Unit-checked both directions on two trees:

identical contents, different on-disk dir sizes -> diff: silent   (would have refused before)
b/d chmod 4777, b/d/f chmod 0755                -> diff: "4777 … d - ./d" and "755 … f 1 ./d/f"   (still bites)

The two real cuts now prove the shape end to end (both exit 0, both from shipped SFS files, not fixtures):

run	input → output	proof
c28	c27 SFS 46cb23d4… → 92db63ac… (shipped) / re-run for the check 7795f82e…	1 path differs (1bit-release-check), metadata 2 lines = its own, repack matches the packed tree
second file	c28 SFS 92db63ac… → 549dcf97…, 489,312,256 B	1 path differs (etc/init.d/10lemonade), metadata 2 lines = its own, repack matches

So the tool composes: one invocation per file, each refusing unless its own single replacement is the only difference.
That is how the next cut carries two files — and it is booted and verified: that image (f7bf6d83…) turns the one red
row green (PASS U3 models dir path=/data/models | models-dir-seeded).

Everything else you verified still holds, including the artifact numbers (46cb23d4… → 92db63ac…, 5b2f5cc0… inside)
and the SFS_JOBS byte-reproducibility limitation noted earlier.

[bong-water-water-bong]

VERDICT: READY at 9286016c (re-review of the manifest mode and the mode field; my previous verdict was at 10e55f2e, before both).

What I ran, at this head:

• patch-sfs-selftest.sh as root, SFS_WORK=/var/tmp/...: 22 passed, 0 failed, rc 0. So the 21 you first reported plus the mode assertion and the non-octal refusal are real, and the fixture cases (single-file mode inherited, setuid elsewhere untouched, manifest with a new path + a changed file + a directory that gains and loses one) all behave.
• bash -n clean on both scripts; patch-sfs.sh is now 100755 in the tree.
• The uid >= 1000 gate, which the selftest explicitly does not cover: I hand-built the SFS it says it needs — a 1000:1000 file outside the declared set — and the run refuses after packing with REFUSED: entries owned by uid or gid >= 1000 rode into the packed SFS, names the three entries, exits 1 and leaves no out.sfs behind. The gate's awk also reads unsquashfs -lln correctly ($2 is uid/gid, verified field by field).
• The mode field, both ways, on the same fixture: a manifest row usr/local/bin/1bit-selftest<TAB>src (no third field) installs mode 775 from the 0775 source; the row ...<TAB>src<TAB>0755 installs 755. So the fix works and the shape that shipped c29 is reproduced exactly.

Two things to consider, neither blocking this merge:

1. An addition with no declared mode still inherits the source's (measured above: 775). That is the omission that produced c29, and the third field is optional, so the class stays reachable by leaving it out. Refusing an addition that has no declared mode — or printing a loud mode <m> inherited from the source line — would close it at the tool instead of at the review. The inherited mode for a replacement is right and should stay: that is what keeps setuid bits.
2. declared() matches by substring (*"$target"*), so a differing path like usr/local/bin/1bit-selftest.bak or opt/1bit-package-extra/x would count as inside the declared set. Nothing the tool does today can produce such a name — every difference comes from a manifest row — so this is only a hole for a future bug, but matching $1 against the target, $target/, and Only in <dir>: <name> would make it strict.

The selftest is not wired into any workflow (nothing under .github/workflows/ touches os/v3/puppy/), which is the same convention as mdns-ports-selftest.sh — an operator script, not a CI row. Noted so nobody assumes CI covers it.

[github-actions]

Preparing review...