Fix the Nix package build and guard it in CI - #182
Conversation
|
Warning Review limit reached
Next review available in: 1 minute You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The Nix package build has been broken since httpz was vendored. Nothing caught it because no workflow ran
nix build.The breakage, as observed
nix/deps.nixis the offline package cache for the Nix build: one entry per Zig package, keyed by the exact.hashfrom a manifest. Three things had gone stale:99df0d3/…ZPISdangBAAg…, while bothbuild.zig.zonandvendor/httpz/build.zig.zonrequireb70e733/…ZPISdUoQBQC7…. The cache therefore never contained the revision zig asks for, which is the error above.vendor/httpznow, so it belongs to the source tree rather than the cache. The entry just fetched a tarball nothing used.nix/package.nixstill saidversion = "0.5.10", two releases behind, so the derivation built aswisp-0.5.10.Verification
Reproduced the failure first, then confirmed the fix, both with a real
nix buildrather than by inspection:package not founderror above, derivation namedwisp-0.5.10nix build .#succeeds, producingwisp-0.6.0Then checked the packaged artifact actually works, since a wrong dependency can still link: ran the store binary and it served NIP-11 reporting
"version":"0.6.0".Also green:
zig build test65/65, andscripts/verify-vendored-httpz.sh.Guarding it
A new
nix package buildjob runsnix build .#and then starts the packaged binary and asserts it answers NIP-11. Building alone would have caught all three problems here, but the runtime check is what distinguishes "linked" from "works".nix flake checkis deliberately not run in CI: the flake'schecks.<system>.wisp-moduleis a NixOS VM test needing/dev/kvm, which standard GitHub runners do not provide. That is noted in the workflow next to the job so the gap is explicit rather than silent.On regenerating deps.nix
The file's header claimed it was generated by zon2nix. It cannot be regenerated: zon2nix fails on our manifest with
error: parseError(verified againstnixpkgs#zon2nix0.1.3), because it cannot express the vendored path dependency. The header now says the file is hand-maintained, records that the entries must cover the union of both manifests, and points at the CI job that catches drift. Anyone following the old header would have hit the parse error and had no idea what to do next.Review follow-ups
Both reviews recomputed every pin against live upstream and found the changed values correct. Four things came out of them:
The
--systemmode comment innix/package.nixwas wrong, and it mattered. It claimed "Zig verifies each against the hash pinned in build.zig.zon". It does not: in--systemmode the hash is a lookup key, and the mismatch comparison lives on the network fetch path that is never reached. I verified this by experiment rather than by reading the resolver: appending a line to a package's source inside the system dir and rebuilding succeeded. So thehash =values indeps.nixare the only integrity gate on this build, and a comment telling a future reviewer otherwise is worse than no comment.Added the cross-check that closes the chain.
scripts/verify-nix-deps.shnow asserts that an entry's URL points at the same revision the manifest asks for, so a substitution changing url and hash together no longer passes silently. Combined with nix's fixed-output hash (url to content) and the networkzig buildin the other jobs (manifest hash to content), manifest to compiled bytes is covered. Mutation-tested against three cases, all caught:package.nixversion driftIt runs before nix is installed, needs no network, and takes about a second.
My justification for skipping
nix flake checkwas false. I wrote that the runners lack/dev/kvm. They do not: the installer enables it, and this PR's own run logsEnabled KVM. I also confirmed the VM test genuinely passes (checks.x86_64-linux.wisp-modulegreen locally), so it is a cost decision, not a capability one. The comment now says that, and the decision is tracked separately rather than being settled by a wrong premise.Hardened the job's failure paths. Under
bash -ea failingcurlaborted the step before the relay log was dumped or the process stopped, and an unguardedkillcould fail the step after a successful check if the relay had already exited. It now uses the samestop()helper with a SIGKILL fallback as the other eight relay steps, captures curl's status explicitly, and carriestimeout-minutes: 20since no job in this repo sets one.Remaining findings are filed rather than fixed here: pinning two dependencies by commit instead of by mutable tag (verified zero-churn for libnostr-z, but it has to move in lockstep with
build.zig.zonor the new cross-check trips),set_as_trusted_user: falseplus the unverified installer bootstrap, and mode/symlink blindness inverify-vendored-httpz.sh. None are defects in this diff.