Repository navigation
feat(bundle): Take the guest initrd from hull-assets - #30
Conversation
9525018 to
8b716e2
Compare
|
@claude-fleet review |
1 similar comment
|
@claude-fleet review |
|
Review task queued. Task: c0c3b0b2-2778-49a0-bd0f-1b42e04d1cf6 |
|
Review task |
![claude-fleet[bot]](https://avatars.githubusercontent.com/in/4876296?s=60&v=4){kind=small}
There was a problem hiding this comment.
Takes the kernel and initrd from one signed hull-assets digest and replaces the local urunit/initrd build with a coherence guard. The shape is right, the docs track the diff, and both new test files pass. Two things to fix before merge, both about the urunit half of the guard.
Findings:
- should_fix, scripts/check-assets.sh:45 — the urunit check fails open on any
sources.urunitlayout the sed does not model - should_fix, scripts/build-bundle.sh:1222 — the shipped
bundle.jsonasserts a urunit commit nothing verified - nit, scripts/build-bundle.sh:1015 —
URUNIT_REPOfrom the pin is no longer validated before it lands inpins.env
Verified:
- assets built from another urunc are refused, with the message the description quotes — scripts/check-assets.sh:38-42;
tests/check-assets.shcase 2 URUNIT_REF,URUNIT_BRANCHandURUNIT_REPOare refused likeHVI_REF— scripts/build-bundle.sh:145-147;tests/source-pins.sh:157-170URUNIT_REFcomes from urunc'spackaging/container-initrd/urunit.pinatURUNC_REF, full-SHA validated — scripts/build-bundle.sh:1011-1023- a gzipped
container-initrdis still rejected — scripts/build-bundle.sh:1211-1212 - docker is now reachable only from the urunc build and the TMP_DIR reclaim — the only
docker runsites left are scripts/build-bundle.sh:254 and :993 SHA256SUMSstill covers the kernel andcontainer-initrd— scripts/build-bundle.sh:1227- nothing in
install.shreadsshare/guest/bundle.json— no hit forbundle.jsonin install.sh --variant stockis untouched: both new blocks are under[ "$VARIANT" != "stock" ]— scripts/build-bundle.sh:1011, :1188- the README now says a
URUNC_REF=tip build fails unlessASSETS_VERSIONnames a matching hull-assets release — README.md:336-338, which matches scripts/check-assets.sh:40
Checks run:
sh tests/check-assets.sh-> 4 oksh tests/source-pins.sh-> 7 oksh -nover install.sh, build-bundle.sh, check-assets.sh and both new tests -> okscripts/check-assets.shagainst a hand-writtenbundle.jsonwhosesources.urunithas a nested object before"ref"-> exit 0, "records no urunit commit", despite a urunit ref that is not the pinned one- no revert check: this PR changes where a file comes from rather than fixing a bug, so there is no regression test to falsify
Smaller things:
tests/source-pins.sh:121:if grep -qv "make static" "$T/docker.log"only catches a wrong docker invocation. It does not assert the urunc build ran, so an emptydocker.logpasses the step that is meant to prove docker is still used once. Addinggrep -q "make static" "$T/docker.log"alongside it closes that.- The initrd no longer gets an explicit
chmod 0644(the oldcppath had one). If hull-assets' layer carries the exec bit, scripts/build-bundle.sh:1514 promotescontainer-initrdto 0755 in the tarball. Cosmetic, but it changes the packed mode silently. - README.md:286 states the initrd is urunit
48a61bdas fact; that is the same unverified claim as thebundle.jsonfinding below.
Not checked:
- the real
ghcr.io/nofireai/hull-assetsmanifests: whether the layer titles are exactlybzImage/Imageandcontainer-initrd, and whetheroras resolvereturns an image manifest rather than an index, which the new CIjq '.layers[]'step assumes. No network here. - hull-assets 0.1.11's actual
bundle.jsonkey layout, and whether its initrd really is urunit48a61bd. - whether brig main reads
share/guest/bundle.json; onlyinstall.shand this repo's scripts were searched. .github/workflows/release.ymlbeyond the two comment edits.
Not checked:
- The real ghcr.io/nofireai/hull-assets manifests: layer titles (bzImage/Image, container-initrd) and whether
oras resolveyields an image manifest rather than an index, both assumed by the newjq '.layers[]'step in .github/workflows/ci.yml:133-141. No network in this environment. - hull-assets 0.1.11's actual bundle.json key layout, and whether its initrd is in fact urunit 48a61bd built against urunc 3c00b69.
- Whether brig main (4fed3cc) reads share/guest/bundle.json; only install.sh and this repo's scripts were searched.
- .github/workflows/release.yml beyond its two comment edits.
Task c0c3b0b2-2778-49a0-bd0f-1b42e04d1cf6 on rp1, model claude-opus-5, effort medium.
| release built from urunc $urunc_ref, or move URUNC_REF_DEFAULT to $got." | ||
|
|
||
| got_urunit="$(source_ref urunit)" | ||
| if [ -n "$got_urunit" ]; then |
There was a problem hiding this comment.
should_fix: An unreadable sources.urunit.ref is treated as an absent one, so the urunit half of the guard silently becomes a no-op instead of failing the build.
Failure: a hull-assets bundle.json whose urunit entry is {"repo": "...", "build": {"arch": "amd64"}, "ref": "deadbeef..."} carries a urunit that is not the one urunc pins, and the build accepts it. I ran scripts/check-assets.sh on exactly that file with urunc-ref matching and urunit-ref = 48a61bd: exit 0, stderr check-assets: ... records no urunit commit; matched on urunc ... alone. The [^}]* in source_ref (line 35) cannot cross the nested }, so the ref is invisible. Any key rename or added sub-object on hull-assets' side has the same effect, and nothing in CI would notice, because the whole point of the branch is that the ref is sometimes legitimately missing.
The urunc branch gets this right (line 39 dies on an empty read). Mirror it: treat "the \"urunit\" key is present in the json" and "its ref parsed" as separate facts, and die when the first holds and the second does not. For example grep -q '"urunit"[[:space:]]*:' "$json" before the if, and die with "records a urunit source whose ref could not be read" when $got_urunit is empty.
| # from. ref and urunit keep their meaning: the urunc the initrd matches and | ||
| # the urunit commit that urunc pins. | ||
| cat > "$g/bundle.json" <<JSON | ||
| {"ref": "$URUNC_REF", "urunit": "$URUNIT_REF", "built_by": "hull-assets", "packed_by": "brig-build-bundle", "kernel": {"file": "$kernel", "source": "$KERNEL_SOURCE"}, "initrd": {"file": "container-initrd", "source": "$KERNEL_SOURCE"}} |
There was a problem hiding this comment.
should_fix: The bundle.json packed into the tarball states "urunit": "$URUNIT_REF" next to "built_by": "hull-assets", but $URUNIT_REF comes from urunc's urunit.pin (line 1018), not from the assets, and against hull-assets 0.1.11 nothing confirms the initrd was built from it.
Failure: hull-assets cuts a 0.1.11-style release from urunc 3c00b69 but a different urunit. check-assets.sh passes on urunc alone (scripts/check-assets.sh:48-50 logs "records no urunit commit"), and the bundle ships claiming urunit: 48a61bd as hull-assets' own record. Someone debugging an agent-protocol mismatch reads that file, sees a matching urunit, and rules out the actual cause. Before this change the field was true by construction, because the build compiled that urunit itself.
Either name the field for what it is (the commit urunc pins, e.g. urunit_expected), or emit it only when check-assets.sh actually read a urunit ref from the signed bundle.json -- it already prints the urunc ref it read, so it could print the urunit one the same way.
| [ -d "${src:-}" ] || fatal "the urunc checkout is needed to read its urunit pin (build urunc from source)" | ||
| pin="$src/packaging/container-initrd/urunit.pin" | ||
| [ -f "$pin" ] || fatal "$URUNC_REPO@$URUNC_REF has no packaging/container-initrd/urunit.pin" | ||
| URUNIT_REPO="$(sed -n 's/^URUNIT_REPO=//p' "$pin" | head -1)" |
There was a problem hiding this comment.
nit: URUNIT_REPO is now read from urunit.pin and written straight to pins.env (line 1476) with the <owner>/<repo> validation dropped along with the checkout.
Failure: a urunc commit whose pin says URUNIT_REPO=git@github.com:NOFireAI/urunit.git yields URUNIT_REPO=git@github.com:NOFireAI/urunit in pins.env. install.sh:638 parses that file with awk -F=, so a value containing = is also truncated on read. No shell sourcing is involved, so this is a record-quality issue rather than an injection one -- the old check and its test (tests/source-pins.sh case 1c, removed here) caught it. URUNIT_REF kept its validation at lines 1019-1022; the repo did not.
8b716e2 to
e21f1fc
Compare
|
@claude-fleet review |
|
Review task queued. Task: 7d33a9fa-f9dc-4a37-bfa8-233fa0fa478e |
There was a problem hiding this comment.
Previous round at 8b716e2: 3 findings.
| Previous finding | Classification | Cited line |
|---|---|---|
scripts/check-assets.sh:45 (should_fix): An unreadable sources.urunit.ref is treated as an absent one, so the urunit half of the guard silently becomes a no-op instead of failing the build. Failure: a hull-assets bundle.json whose urunit entry is {"repo": "...", "build": {"arch": "amd64"}, "ref": "deadbeef..."} carries a urunit that is not the one urunc pins, and the build accepts it. I ran scripts/check-assets.sh on exactly that file with urunc-ref matching and urunit-ref = 48a61bd: exit 0, stderr check-assets: ... records no urunit commit; matched on urunc ... alone. The [^}]* in source_ref (line 35) cannot cross the nested }, so the ref is invisible. Any key rename or added sub-object on hull-assets' side has the same effect, and nothing in CI would notice, because the whole point of the branch is that the ref is sometimes legitimately missing. The urunc branch gets this right (line 39 dies on an empty read). Mirror it: treat "the \"urunit\" key is present in the json" and "its ref parsed" as separate facts, and die when the first holds and the second does not. For example grep -q '"urunit"[[:space:]]*:' "$json" before the if, and die with "records a urunit source whose ref could not be read" when $got_urunit is empty. |
fixed | scripts/check-assets.sh:50 |
scripts/build-bundle.sh:1222 (should_fix): The bundle.json packed into the tarball states "urunit": "$URUNIT_REF" next to "built_by": "hull-assets", but $URUNIT_REF comes from urunc's urunit.pin (line 1018), not from the assets, and against hull-assets 0.1.11 nothing confirms the initrd was built from it. Failure: hull-assets cuts a 0.1.11-style release from urunc 3c00b69 but a different urunit. check-assets.sh passes on urunc alone (scripts/check-assets.sh:48-50 logs "records no urunit commit"), and the bundle ships claiming urunit: 48a61bd as hull-assets' own record. Someone debugging an agent-protocol mismatch reads that file, sees a matching urunit, and rules out the actual cause. Before this change the field was true by construction, because the build compiled that urunit itself. Either name the field for what it is (the commit urunc pins, e.g. urunit_expected), or emit it only when check-assets.sh actually read a urunit ref from the signed bundle.json -- it already prints the urunc ref it read, so it could print the urunit one the same way. |
fixed | scripts/build-bundle.sh:1228 |
scripts/build-bundle.sh:1015 (nit): URUNIT_REPO is now read from urunit.pin and written straight to pins.env (line 1476) with the <owner>/<repo> validation dropped along with the checkout. Failure: a urunc commit whose pin says URUNIT_REPO=git@github.com:NOFireAI/urunit.git yields URUNIT_REPO=git@github.com:NOFireAI/urunit in pins.env. install.sh:638 parses that file with awk -F=, so a value containing = is also truncated on read. No shell sourcing is involved, so this is a record-quality issue rather than an injection one -- the old check and its test (tests/source-pins.sh case 1c, removed here) caught it. URUNIT_REF kept its validation at lines 1019-1022; the repo did not. |
fixed | scripts/build-bundle.sh:1006 |
All three findings from the last round are fixed, and I proved each fix's test fails without it. Approve.
Verified:
- Unreadable
sources.urunit.refnow refused, not treated as absent — scripts/check-assets.sh:50-51; the"urunit"presence grep is separate from the ref parse, matching the urunc branch's fail-closed shape. bundle.jsonno longer claims hull-assets recorded a urunit it did not:check-assets.shreturns<urunc> <urunit|->(check-assets.sh:57) and build-bundle.sh:1227-1231 emitsurunit_expectedonly in the-case, with the comment saying nothing checked it against the initrd.URUNIT_REPOvalidated before it reachespins.env— scripts/build-bundle.sh:1005-1010;git@github.com:...hits the*[!A-Za-z0-9._/-]*arm, empty hits the final*).- README:286 and README:337 match the code:
URUNC_REFis reassigned to the resolved commit at build-bundle.sh:987 beforecheck-assets.shcompares, so aURUNC_REF=tip build does fail unlessASSETS_VERSIONwas built from that tip. - No consumer of the dropped
URUNIT_PINNED/URUNIT_BRANCH/INITRD_SOURCE=built:keys:grepover install.sh and the tree finds none.
Checks run:
sh tests/check-assets.sh→ 5 oksh tests/source-pins.sh→ 8 ok- Removed the
elif ... grep -q '"urunit"...'branch (check-assets.sh:50-51) →tests/check-assets.shcase 3b fails:FAIL: a urunit source the check cannot read was accepted. Restored. - Removed the
case "$URUNIT_REPO"block (build-bundle.sh:1005-1010) →tests/source-pins.shcase 1c fails:FAIL: a urunit.pin with an ssh URL was not refused, log showspins git@github.com:NOFireAI/urunit@3333.... Restored;git status --porcelainclean both times.
Smaller things:
- No test covers the
urunit_expectedvsurunitbranch in the packedbundle.json(build-bundle.sh:1227-1231). The CIjq -eonly assertsbuilt_byand the twosourcefields, so a swap of the two arms would go unnoticed. Cheap to addand has("urunit_expected")there while hull-assets 0.1.11 is the pin. docs/variants.md: "hull-assets' own build checks that urunit is built for the target architecture" is a claim about another repo; the brig-side arch check is gone and nothing in this tree replaces it. The per-arch tag plus the signed digest keeps it narrow.
Not checked:
- hull-assets' own bundle.json layout and whether its build checks urunit's target architecture (external repo, not reachable here)
- That a hull-assets layer digest equals the file's sha256, which the new CI manifest check at .github/workflows/ci.yml:131-139 assumes (needs network and oras)
- The CI bundle job and release job end to end; only read, not run
Task 7d33a9fa-f9dc-4a37-bfa8-233fa0fa478e on rp1, model claude-opus-5, effort medium.
| # HEAD answers with it, or with $STUB_HEAD, or with $tip for a branch. A | ||
| # checkout of the urunc source plants urunit.pin, as the real commit carries | ||
| # one; STUB_NO_PIN=1 leaves it out, and STUB_PIN_REPO replaces its repo. | ||
| # one; STUB_NO_PIN=1 leaves it out. |
There was a problem hiding this comment.
nit: The stub git's comment no longer mentions STUB_PIN_REPO, which the stub still reads and case 1c still sets.
Failure: a reader of tests/source-pins.sh:34-37 sees only STUB_NO_PIN=1 leaves it out and has to find tests/source-pins.sh:62 ("${STUB_PIN_REPO:-https://github.com/NOFireAI/urunit.git}") and tests/source-pins.sh:139 to learn the knob exists.
Evidence: the sentence "and STUB_PIN_REPO replaces its repo" was dropped in the commit that removed case 1c, and the knob came back in e21f1fc without it. Restore that clause.
Since hull-assets 0.1.11 its initrd is urunit's single binary, built from the urunit commit that urunc 3c00b69 pins. That is the urunc the bundle builds. The bundle now keeps hull-assets' container-initrd next to its kernel and stops building urunit and its own initrd. - scripts/check-assets.sh reads sources.urunc.ref from the signed hull-assets bundle.json and fails the build unless it is URUNC_REF. hull-assets records no urunit commit yet. When it does, the check requires the commit urunc pins in its urunit.pin. - The build still reads urunit.pin from the urunc checkout and records URUNIT_REPO and URUNIT_REF in pins.env. URUNIT_BRANCH and URUNIT_PINNED are gone, and setting URUNIT_REF, URUNIT_BRANCH or URUNIT_REPO is refused. - share/guest/bundle.json keeps ref and urunit, sets built_by to hull-assets and names the digest each file came from. INITRD_SOURCE in pins.env is that digest too. - CI checks that the kernel and the initrd are the layers of the hull-assets manifest pins.env names, and that ASSETS_URUNC_REF is the pinned urunc. - tests/check-assets.sh covers the check. tests/source-pins.sh now fails at the first download and checks that docker runs only for the urunc build. Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
The README, DESIGN.md and docs/variants.md said the build assembles its own initrd from urunit. They now say the kernel and the initrd come from one signed hull-assets bundle, and that the build refuses a bundle built from another urunc. Signed-off-by: Anastassios Nanos <ananos@nofire.ai>
e21f1fc to
331f478
Compare
Takes the guest initrd from the signed hull-assets bundle, as the bundle already does for the kernel, for v0.1.0-rc19. Stacked on #29 (hull-assets 0.1.11).
Since hull-assets 0.1.11 its initrd is urunit's single binary (
/urunitwith/init,/vz-initand/urunit-agentlinks), built against urunc 3c00b69. That is this bundle'sURUNC_REF_DEFAULT, and urunc'surunit.pinthere names urunit 48a61bd. So the bundle no longer builds urunit or its own initrd.Changes
scripts/build-bundle.sh: drops the urunit checkout and build and thebuild-container-initrd.shrun. It keeps hull-assets'container-initrdand still rejects a gzipped one. docker is now needed only for the urunc build.--variant stockis unchanged: it has no urunit, kernel or initrd.scripts/check-assets.sh(new): the coherence guard. It readssources.urunc.reffrom the hull-assetsbundle.json(pulled from the cosign-verified digest) and fails the build unless it equalsURUNC_REF. hull-assets 0.1.11 does not record a urunit ref. When it does, the guard requires the commit urunc'surunit.pinnames; until then it logs that it matched on urunc alone.URUNIT_REFinpins.envcomes from urunc'spackaging/container-initrd/urunit.pinatURUNC_REF, as before, since hull-assets'bundle.jsondoes not carry it.URUNIT_BRANCHandURUNIT_PINNEDare gone. SettingURUNIT_REF,URUNIT_BRANCHorURUNIT_REPOis refused, likeHVI_REF.INITRD_SOURCEis the hull-assets digest, andASSETS_URUNC_REFis what hull-assets recorded.share/guest/bundle.jsonkeepsrefandurunit, setsbuilt_bytohull-assets, addspacked_by: brig-build-bundleand names the digest each file came from. Nothing in install.sh or brig main (4fed3cc) reads this file.SHA256SUMSstill covers the kernel andcontainer-initrd.pins.env, thatbundle.jsonnames it, and thatASSETS_URUNC_REFis the pinned urunc.URUNIT_PINNEDis no longer checked.tests/check-assets.sh.tests/source-pins.shnow ends at the first download and checks that docker runs only for the urunc build.Tested
Locally (macOS, no docker):
sh -nand shellcheck 0.11.0 over the CI file list, and everytests/*.sh. The Linux-only install tests skip on macOS.scripts/check-assets.shagainst the real hull-assetsbundle.jsonfiles:CI builds the four bundles. Pending: the same release gates as #29 on this PR's artifacts.
Release sequence
Merge #29, rebase this onto main, merge, then tag
v0.1.0-rc19on main.🤖 Generated with Claude Code