Conversation
wesleygrimes
left a comment
There was a problem hiding this comment.
Direction and sequencing look right: no template policy change, no strict migration, version stays rc4, and the build contract now enforces the versioned keyring dependency to match omarchy-mac/omarchy-pkgs-aarch64#28. Verified the committed keyring binaries: base quattro holds only F3C5AE3F…/6C2597C6…, this branch holds only FBD6874D…/D791ED0C….
One cross-PR note: #436 still pins the old primary and subkey in 1789317000.sh, the docs, and its test fixture, so it needs a rebase onto this before it can merge.
Inline comments cover the remaining items: the apparently redundant 1789390468.sh, a hardcoded developer path in the native test, the upgrade test being skipped in hosted CI, a doc note on why the key changed and that upgraded hosts keep both keys, and the strict-policy canary edge case.
scottjones
left a comment
There was a problem hiding this comment.
Reviewed against quattro, ran the touched suites locally, and read Wesley's review.
CI is green (7/7). All six touched/added shell suites pass here — the one package-build-contract-test.sh failure I hit was a missing sibling omarchy-pkgs checkout, environmental, and it passes once linked.
Direction and sequencing look right to me too. One improvement worth naming explicitly: in omarchy_arm_channel_trust_fork the fingerprint verification now runs unconditionally, where the old code only verified when the key had just been added. A keyring that already held a wrong key under that name would previously have been lsigned without a check. Good change.
I agree with all five of Wesley's items. Two I've sharpened in-thread:
- The skipped native upgrade test is worse than "only exists locally" — the pinned baseline commit is not in the repo at all (GitHub returns 422 for it), so the opt-in flag cannot work for anyone but the author.
- Dropping
1789390468.shalso removes a live bug: it usesreturnat script top level, but migrations are executed rather than sourced.
Four further items inline. None of them block on their own, but the first is a rollout-ordering hazard I'd want settled before publication.
Cross-PR: confirmed #436 still pins F3C5AE3F… in 1789317000.sh, so Wesley's rebase note holds.
|
Review follow-up at Resolved as implemented and verified: redundant/broken The native keyring test passed directly in the Arch ARM UTM VM for uninitialized, fresh-install, and real Left open intentionally as non-blocking follow-ups: consolidating the active fingerprint constant and simplifying/renaming the dependency parser. |
Required merge and rollout orderThe PR dependency order is:
Publication gates are part of that order; merging the source PRs alone is not sufficient:
In short: #28 → publish and verify RC4 bootstrap artifacts → #444 → verify deployed bootstrap → prepare and canary signed RC5 → #436. Do not merge |
scottjones
left a comment
There was a problem hiding this comment.
Re-reviewed at 1b117466. Verified the fixes rather than taking them on trust:
115's new gate behaves as described — against an installed20260913-1it now exits1withOmarchy Mac keyring 20260914-2 or newer is required (installed: 20260913-1); …and makes nopacman-keycall, where the previous version aborted the whole queue silently. The nine-scenario queue test coversold-onlyandinstall-old, which is exactly the publication-ordering hazard.1789390468.shis gone, and both suites now assert against its top-levelreturnerror rather than just a non-zero exit.- Fixture checksums verify clean; the test runs with no
.gitpresent. - The preflight comment is back above
omarchy_arm_channel_prepare.
All eight suites this PR touches pass here, and CI is green on the exact head. For the two failures in my full-suite run (install-vm-source-isolation, plymouth-set): they reproduce identically on unmodified quattro under the same environment, so they are my local setup, not this branch. The five test files this PR adds all pass.
I also want to note the handling of the hosted-CI item: removing the nspawn wiring and adding assertions that stop that job from re-claiming the coverage is a better outcome than leaving it in place and calling it covered. Same for marking the install.sh constant and the build-packages.sh parser as deferred rather than folding them in silently — no objection to either deferral, neither was a correctness issue.
One small follow-up inline, not a blocker.
Finalize the RC4 bootstrap keyring before enabling signed RC5. Ship only primary
FBD6874D423C418DDB6D143EECE19CDDE306DBD2inomarchy-mac-keyringversion20260914-2, matching omarchy-mac/omarchy-pkgs-aarch64#28. Fresh installation and private channel trust use the new primary; previously installed old and unrelated trust is preserved.The version stays
4.0.3rc4. The four repository templates and channel policy renderer retain the reviewed RC4 behavior, including preservation of stricter existing policies. The strict RC5 migration is absent.Review cleanup at
07753c84583aee5f112adc3d74bacdb300a7be0d:20260914-2before its explicit trust population and reports package/version, population and fingerprint failures. The existing package helper still receives a plain package name; its skip-success case is checked explicitly.1789390468.shand its invalid top-levelreturn. Retain1789407944.shfor clients with completed earlier markers. Failed migrations remain pending and stop the queue.The fingerprint-constant refactor and dependency-parser simplification/naming suggestions remain deferred. All nine review conversations have responses describing what was changed, what remains unverified, or why an item was deferred; replies do not imply reviewer approval or conversation resolution.
Validation at the cleanup:
.git: uninitialized trust, fresh installation, and prior-version upgrade preserving old/unrelated trust.update-lockfailure was previously reproduced on the unchanged baseline; this is not reported as a complete local-suite pass.bwrap: loopback: Failed RTM_NEWADDR: Operation not permittedbefore any native scenario executed. Hosted native install/upgrade coverage is still unresolved. No host protection was relaxed and no test was silently skipped.Earlier qualification archives built from
55542531predate the review cleanup and are historical evidence, not publishable artifacts of the current head. No production signing secrets, host trust stores, tags or releases were changed. Publication still requires a reviewed current inventory, correct archive reuse and actual fresh/existing Mac qualification. Clients must receive the RC4 trust bootstrap before signed RC5 or edge conversion.