[bong-water-water-bong]

Follow-up to #1352 (closed by #1361). No issue to close — this is the same defect class in a site both earlier fixes missed, found by enumerating the packing sites instead of reasoning about the two already fixed.

What was still open

Three scripts source scripts/pack-elf.sh and write an archive. Only two applied the mode rule:

site	output	mode guard
package-linux.sh	engine package	fixed in #1361
weekly.sh	the lemonade payload fold	fixed in #1360
make-binary-drop.sh	binary-drop-<version>.tar.zst — a published artifact (docs/releases.md, "The public binary drop")	neither helper called
pack-distro.sh	distro packages	normalises per payload pass; its own comment explains why it does not assert

Measured, not argued

make-binary-drop.sh stages with cp -a from whatever it is pointed at:

• --build <dir> → the build tree's modes. Measured on a real one, ~/wt/rlk-build/hrx/llama/bin is 13 of 13 files at 0775 (-rwxrwxr-x) — so a drop built that way ships 0775 binaries and libraries.
• --from-package <tarball> → whatever that tarball had, including one packed before package-linux: a staged file's mode is the packer's decision, and the package asserts it #1361/package: the lemonade payload is the second packing site, and it shipped 0775 files (#1352) #1360. c31's payload measured 111×0664 + 23×0775.

So the published drop's modes were entirely inherited from its inputs. It now runs pack_normalise_modes and asserts with pack_writable_files before writing the archive, and docs/releases.md says so in the drop's own section.

The regression guard, and why it looks the way it does

tests/pack_modes_test.sh now enumerates the packing sites from the tree (sources pack-elf.sh + writes an archive) rather than listing them, so a fourth site cannot be forgotten. Two tiers, because the sites are not the same job:

• every site that stages files must call pack_normalise_modes — no exceptions, it cannot change behaviour;
• the sites that pack a flat payload must also assert with pack_writable_files.

pack-distro.sh is the single exemption, and it is that script's own decision, documented at its lines 63-74: it stages on the formatter's own copy, so the chmod cannot fail there, and it judged the reporter to be decoration. The test records that decision explicitly, so a second site wanting the exemption has to edit the list — which is the point.

Three negative controls, run and restored

control	result
normalise removed from the drop	rc 1, names the site
only the assert removed (normalise kept)	rc 1, names the site
the exempt site loses its normalise calls	rc 1, names it

The first version of this check matched a commented-out call and passed with the guard disabled — caught by the negative control, which is why the pattern now anchors at the start of a line. That is the second time in this work that a check I wrote could not fail; both times the control is what found it.

One observation I am not overriding: pack-distro.sh's comment says the mode is "checked where it can actually be wrong … on the finished archives, by scripts/pack-proof.sh". pack-proof.sh compares modes between formats (deb/rpm/arch agree), so it cannot catch a uniformly-writable payload — the exemption rests on "the chmod cannot fail", not on a proof. Flagging it for that lane rather than changing their call.

Checks: bash tests/pack_modes_test.sh rc 0 (plus the three controls above), bash -n clean on the touched script, tools/copyright.py --check rc 0. 0 s GPU, 0 s NPU, no ticket, nothing merged.

[bong-water-water-bong]

#1435 ready for review at 4bb2df2

Residue of #1352, found by enumerating the packing sites: make-binary-drop.sh sources scripts/pack-elf.sh, writes a published tarball, and called neither mode helper. Measured on a real tree: ~/wt/rlk-build/hrx/llama/bin is 13 of 13 files at 0775, which is exactly what --build stages with cp -a; --from-package inherits an older tarball's modes (c31: 111×0664 + 23×0775).

Fix is the same pair #1361 introduced, applied before the archive, plus docs in the drop's own section. The regression guard enumerates the packing sites from the tree (sources pack-elf.sh + writes an archive) so a fourth cannot be forgotten: every site must call pack_normalise_modes; the flat-payload sites must also assert with pack_writable_files; pack-distro.sh is the one documented exemption, recorded in the test.

Three negative controls run and restored (rc 1 each; the first version of the check matched a commented-out call and passed with the guard disabled — the control caught it).

bash tests/pack_modes_test.sh rc 0, bash -n clean, copyright.py --check rc 0. 0 s GPU, 0 s NPU.

[bong-water-water-bong]

/review

[github-actions]

Preparing review...

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit ea0d708)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶 1352 - Partially compliant Compliant requirements: The make-binary-drop.sh script now normalizes file modes and asserts that no files are group- or other-writable The tests/pack_modes_test.sh script now enumerates packing sites and validates that all relevant scripts apply the mode normalization The documentation in docs/releases.md has been updated to reflect the new behavior Non-compliant requirements: None Requires further human verification: None

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Missing mode normalization The make-binary-drop.sh script was missing the mode normalization step that ensures file modes are not inherited from inputs. This could lead to group- or other-writable files being included in the published binary drop, which violates the security and consistency requirements. # ── the modes, then the tarball and its SHA256SUMS ────────────────────────────────────────────── # A file mode in the drop is THIS script's decision, not its inputs' (engine#1352). That matters more here than in # the two sites already fixed: --build stages straight out of a build tree, and measured on a real one # (rlk-build/hrx/llama/bin) all 13 files are 0775, so a drop built that way ships 0775 binaries; --from-package # inherits whatever an already-published tarball had, including one packed before #1361/#1360. Measured, not assumed: # the same class shipped 134 of 195 files writable in c31 and c32. pack_normalise_modes "$stage" modes_bad=$(pack_writable_files "$stage") [ -z "$modes_bad" ] || die "files still group/ or other-writable after normalising: $modes_bad"

✅ Resolved findings

tests/pack_modes_test.sh:94-131

Enumeration of packing sites

The test now enumerates packing sites from the tree instead of listing them, which helps prevent future regressions by ensuring that all scripts that stage files with pack-elf.sh and write an archive are checked for proper mode handling.

[bong-water-water-bong]

VERDICT: READY at 4bb2df21bfbab888d6d2903951a11c630e6f4860

Finding the third site by enumerating packing sites, rather than reasoning from the two already fixed, is the right method, and the fix is the same two calls the other sites use:

• pack_normalise_modes "$stage", then pack_writable_files asserted, both before the tar -C "$work" -cf - "$name" | zstd … at make-binary-drop.sh:228;
• docs/releases.md says so in the drop's own section.

What I ran at this head (base 880c9ae, on main):

• bash tests/pack_modes_test.sh .: pack_modes: ok. It enumerates four sites: make-binary-drop.sh, package-linux.sh and weekly.sh "normalises and asserts", and pack-distro.sh "normalises (…see its own comment)";
• bash -n passes on both files;
• shellcheck -S warning on the two files reports 1, the same 1 as on main;
• tools/copyright.py --check: rc 0.

The ^[[:space:]]* anchor, which stops a commented-out call from counting, is exactly the "check that cannot fail" trap, and your negative control found it. Good.

Two small robustness notes on the enumeration, not blocking:

• The predicate is "contains tar -C", not "writes an archive". In weekly.sh both matches are extracts (:332, :486 -xf), and make-binary-drop.sh:95 is an extract too. Over-inclusion is the safe direction. Under-inclusion is the risk: a site that writes tar --zstd -cf "$out" -C "$dir" … (flags before -C) won't match.
• The floor is >= 3 while 4 sites are known today. So exactly the case above (one of the four dropping out of the enumeration) passes silently. >= 4 keeps your "a fifth site with the guard is fine" property and catches one disappearing. A predicate like grep -qE '\btar\b.*-c' (or a -cf/--create match) would select writers rather than extractors.

[bong-water-water-bong]

Attempted the end-to-end run of this site tonight — could not complete it, and the reason is worth recording

The fixture test proves the functions; it does not prove this call site executes correctly inside the real drop. So I tried to produce a real binary-drop-*.tar.zst on this box, twice, and both attempts were refused by the drop's own checks before reaching the staging that my change touches.

scripts/make-binary-drop.sh <checkout> <outdir> --build <dir> --npu-lib <libnpu_rt_api.a> requires a release-shaped hrx/llama/bin whose libggml-hrx.so really carries the private HIP kernels (nm for ggml_hrx_hip_addon_register, strings for amdgcn-amd-amdhsa) and whose libraries all resolve. The two candidate trees on this box each fail one half:

tree	has HIP code objects	release-shaped / libs resolve
~/wt/rlk-build/hrx/llama/bin (my #751 build)	no — amdgcn-amd-amdhsa count 0	yes (27 files, drop ran the engine packer from it cleanly)
~/wt/tprs-hrx-154/bin (another lane's partial lib dir)	yes — count 2	no — ldd ./llama-server reports 10 not found; no libggml-hrx.so symlink

So: not verified end-to-end here. What would settle it is a drop built from a real release build — the script's own usage says WEEKLY_DRY_RUN output is fine — and failing that, --from-package <engine tarball> against a package that carries the private kernels. Any of those is a pi-release product, not something this lane can fabricate without a full ONEBIT_GPU_PRIVATE build.

I am recording this rather than leaving the fixture test to imply more coverage than it has. What is covered:

• the two helpers at the heart of it, proven end-to-end through the real scripts/package-linux.sh on a real build tree: pre-fix artifact 30 writable entries → 0, 14 symlinks preserved, 19 executables unchanged (package-linux: a staged file's mode is the packer's decision, and the package asserts it #1361's verification comment has the table);
• this script's call site, only at the unit level: tests/pack_modes_test.sh asserts that every packing site which sources pack-elf.sh and writes an archive invokes pack_normalise_modes, and that flat-payload sites also assert with pack_writable_files, with negative controls (rc 1) for each tier.

The residual risk if the call site were wrong is small and bounded: it sits immediately before tar, after every staging line, and its two calls are the same pair already exercised by the packer — but "small and bounded" is not "verified", and the difference is the point of writing this down.

Commands, for whoever has a release build to hand:

scripts/make-binary-drop.sh <checkout> /tmp/dropout --build <release build> \
  --npu-lib <libnpu_rt_api.a> --version x.y.z
~/wt/pkgaudit/verify_pkg.sh /tmp/dropout/binary-drop-x.y.z.tar.zst   # expects 0 writable, symlinks intact, executables kept

[bong-water-water-bong]

VERDICT: READY at ea0d70852bae382d36966e65a807691cfad0bd96

This head adds only a merge of main on top of 4bb2df21, which I reviewed READY. The verdict carries forward on these checks:

• First parent is 4bb2df21, the reviewed head.
• Own diff is unchanged: patch-id 10c387c4, the same before and after the merge.
• Main didn't touch the PR's files. The only main commit brought in is #1439 (tools/hyperloom/custom/*), which touches none of scripts/make-binary-drop.sh, tests/pack_modes_test.sh or docs/releases.md.
• Its test passes at this head: bash tests/pack_modes_test.sh . → pack_modes: ok.

My two notes from the earlier review (the tar -C predicate and the >= 3 floor) still stand as suggestions.

[github-actions]

Persistent review updated to latest commit ea0d708

[bong-water-water-bong]

End-to-end verification — now measured, both arms, on real drop artifacts

Earlier on this PR I recorded that this fix's call site was only covered at unit level, because every attempt to drive make-binary-drop.sh was refused by a different check. That blocker is gone: main's code-object check now reads e_machine instead of grepping for amdgcn-amd-amdhsa, so the path runs end to end. Both arms below are real make-binary-drop.sh runs from a real engine tarball, with only the mode block changed between them.

Source with unnormalised modes (so the comparison means something): the c31 payload's opt/1bit-package/1bit, repacked as an engine tarball — 46 files at 0664, 21 at 0775, 42 dirs at 0775, and it satisfies the drop's own checks (addon symbol present, 54 EM_AMDGPU code objects, 0 unresolved libraries).

	guard off (pre-#1435)	guard on (as merged)
tarball	binary-drop-0.0.0-c31none.tar.zst	binary-drop-0.0.0-c31arm.tar.zst
files / symlinks / dirs	22 / 14 / 6	22 / 14 / 6
file modes	15×0775 + 7×0664	15×0755 + 7×0644
group/other-writable entries	28 — 22 files, the root, and 5 directories (hrx, hrx/llama, hrx/llama/bin, npu, npu/include)	0
executable files	15	15
archive bytes	15,128,482	15,128,333

Three things this settles that the unit test could not:

1. The call site executes correctly inside the real script — not just "the helper works when called": the drop runs its checks, the guard fires after every staging line, and checks: ok is printed with the artifact written.
2. It cannot false-fail the drop. The arm-1 run completed rc 0 with the guard in place, on a source whose modes were deliberately dirty.
3. The writable directories are real here too — five of them in the counterfactual, in the same class as the c31 measurement that started package-linux.sh packs group/other-writable modes (134 of 195 payload files are 0775/0664): normalise at pack time #1352, so normalising -type d as well as -type f was load-bearing rather than defensive.

Also run for completeness: a drop from the post-#1361 package (~/wt/pkgtest/1bit-0.0.0-modestest-linux-x86_64.tar.zst, whose modes are already normalised) also completed and audits at 0 writable — so the guard is not merely rejecting everything, which is the failure mode a gate like this most easily falls into.

Method note: both arms were run in the same tree, sequentially, with the block disabled as a unit — disabling only the assignment leaves [ -z "$modes_bad" ] to die under set -u, which is what my first attempt did.

Harness: ~/wt/pkgaudit/verify_pkg.sh <tarball> unpacks and reports the histogram, pack_writable_files, symlink survival and the executable count, exiting non-zero on anything writable. Artifacts kept at ~/wt/droptest/{c31arm,c31none,e2e}/.