feat(justfile): present CI's testable surfaces as just recipes (remove dep on previous minimal shim) - #787
Conversation
📝 WalkthroughWalkthroughThe PR centralizes build, artifact, CI, VM, e2e, lifecycle, and local bring-up workflows in the ChangesCI parity and integration workflows
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
622f1cd to
b0b4e52
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@justfile`:
- Around line 176-177: Update the Darwin and default build commands in the just
min recipe to use Cargo’s locked dependency resolution, matching the
locked-build contract and other build recipes. Preserve the existing package
selections for cargo build -p minimal and cargo build -p minimal -p minimald.
- Around line 528-545: Update the soak recipe’s Linux case to export the same VM
readiness and autospawn timeout overrides used by the e2e flow, while leaving
Darwin behavior unchanged. Locate the corresponding timeout environment variable
names in the existing e2e recipe and apply their 150-second values before
invoking scripts/soak-session-e2e.sh.
In `@scripts/reap-vms.sh`:
- Around line 10-20: Update the cleanup flow in scripts/reap-vms.sh to attempt
the scoped pkill commands as the current user before retrying them through
passwordless sudo. Preserve the existing ROOT-based process matching and
non-failing behavior, while ensuring user-owned minvmd and gvproxy processes are
still reaped when sudo is unavailable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7bb66591-ad8b-4e5c-94d6-281003c3062a
📒 Files selected for processing (7)
CONTRIBUTING.mdCross.tomldocs/ci-strategy.mdjustfilescripts/minvmd-lifecycle.shscripts/reap-vms.shscripts/session-e2e.sh
b0b4e52 to
9bbd40d
Compare
Give developers the same testable surfaces the CI lanes run (docs/ci-strategy.md §8: workflows stay a thin scheduler; the justfile and scripts/ are the reviewed logic), each recipe's comment naming its CI counterpart: - quick gates: fmt, fmt-check, clippy, deny (--all-features, matching the cargo-deny action), with a _need helper for install hints - core: test (core-tests parity, --profile ci on Linux), doctest, test-ignored (the ungated #[ignore] tests only a dev machine can run), and a `ci` umbrella of the PR gate set - test-cross: clippy + tests for the Linux-only crates from a macOS host via cross/Docker (first run is slow; warm cache after) - integration/e2e: test-vm (archive-replay + codesign-last on macOS, deterministic FFI-smoke binary resolution), test-root-integration, e2e, e2e-native, test-lifecycle (switchless, like CI's lifecycle job), soak, reap Also repair the macOS bring-up recipes: `min` no longer builds the Linux-only minimald on darwin; `artifacts` resolves the shim from ~/.minimal/shim/bin when off PATH, requires it only when a fetch is actually needed, and passes repo-RELATIVE --output paths (the shim-VM materialize syncs outputs through the project overlay — absolute paths, even inside the repo, fail the copy-out); and `up` returns as the documented bring-up alias, dispatching to the host's default run mode (macOS → dm1, Linux → dm2 native). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The bare-name pkill killed collateral: another checkout's live VM, or podman's gvproxy. Persistent leftovers all carry the absolute repo path in their cmdline (minvmd's supervisor and __krun-vmm re-exec via current_exe(); gvproxy is spawned from the full MINVMD_GVPROXY_BIN path), so anchor the patterns to the script's own repo root. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The checkouts crate's tests shell out to `git init -b`, which needs git >= 2.28; the cross image's Ubuntu 20.04 base ships 2.25 and CI runners have a modern git natively, so `just test-cross` was the first context to hit it. Pull git from the git-core PPA, noninteractively (the PPA install drags in tzdata, which otherwise prompts and wedges the image build). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
CONTRIBUTING's pre-PR block becomes `just ci`, and both test-extension contract tables (CONTRIBUTING.md and ci-strategy §10) get the recipe names in their local-command column. The manual run-the-harness-binary fallback now states the env it needs (MINVMD_E2E=1, the MINVMD_* paths, --include-ignored) — without it every test self-skips green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
minimald can reset the very first SSH connect right after binding its socket, which trips the CLI's autospawn and failed the recipe even though the daemon was healthy. Use the same 5x2s retry loop dm3 already has. Verified cold and warm on a Linux host. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On slower Linux hosts (nested KVM) the generic guest kernel spends 40-70s probing hardware before pid-1 starts, overrunning the supervisor's 60s READY default and the CLI autospawn's 75s wait — the e2e's post-stop respawn proof then fails on a healthy stack. Export the same 150s headroom the dm3 recipe already carries, caller- overridable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
minvmd-lifecycle.sh hardcoded `run --detach --timeout 30` and a ~15s Running poll; CI's guests boot well inside both, slower dev hosts (nested KVM) do not. One env knob, MINVMD_LIFECYCLE_BOOT_TIMEOUT_SECS (default 30, so CI is unchanged), now drives both waits, and the test-lifecycle recipe exports 150 on Linux alongside the matching supervisor READY headroom. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
e2e-native's userns preflight told the user to flip kernel.apparmor_restrict_unprivileged_userns=0 host-wide. Advise the per-binary profile the installer ships instead, in the installer's own wording (sudo scripts/install-apparmor-profile.sh --path target/debug/minimald), and treat "profile installed + tunables name this checkout's binary" as remediated — the sysctl stays 1 in that case, so it alone cannot be the gate. The build now precedes the preflight so the advised --path target exists. dm2 gets a warn-only twin: the daemon runs fine restricted, but every activate sandbox dies at uid_map until the profile is in. test-root-integration keeps its sysctl hint: its namespaces are created by hashed-path test binaries and /usr/bin/unshare, which a path-attached profile cannot cover. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
session-e2e.sh parked its state dir on /tmp while deliberately reusing the host package cache — but minimald hardlinks built packages from the cache into each session rootfs under the state dir, and hardlinks cannot cross filesystems. On hosts with a tmpfs /tmp (Fedora's default; common elsewhere) every native (DM2) session died at spawn with EXDEV; CI runners keep /tmp on the root disk, which is why the lane never saw it. On Linux the workdir now lives under $HOME — same device as the cache, the assumption production's ~/.local/state already makes — and doubles as the state root: the spare /state hop plus a shorter native seed template keep the deepest per-task socket (tasks/<seed>-<ts>-<n>-<pid>/run/minenv_sock) inside the 108-byte sun_path budget. macOS keeps its /tmp layout (104-byte limit, VM-backed lanes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Collapse the per-recipe `case $(uname -s)` dispatch into [linux]/[macos] recipe attributes, hoist the repeated MINVMD_*/PATH/timeout exports to top-level `export`, and dedupe the kvm/userns preflights and the first-connect retry into shared helpers. Rename the deployment-model recipes to plain verbs: dm1/dm2 fold into the per-OS `up`, dm3 becomes `up-kvm`, dm2-down becomes `down` (macOS `down` delegates to `stop`). Drop the unreferenced codesign-minvmd release recipe and test-vm's unused gvproxy fetch; resolve the macOS shim by install path so a stale target/debug binary can't shadow it. Comments cut to captions plus landmines; session-e2e.sh drops its DM parentheticals and docs/ci-strategy.md points at up/up-kvm. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
9bbd40d to
ef8f7db
Compare
Every other build recipe passes --locked; these two could silently update Cargo.lock on a dev machine. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
sudo -n fails fast without passwordless sudo and || true swallowed it, so the documented user-owned fallback never ran. Kill as the invoking user first, then escalate for root-owned relay leftovers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
The only sanctioned root step is installing minimald's AppArmor profile (_userns-check checks for it and prompts). Drop the sudo commands from the other preflights: _kvm names the kvm-group requirement without prescribing usermod, and test-root-integration explains why the policy cannot cover its surface instead of advising a host-wide sysctl flip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The per-commit package map (buildbot's commit_index) only exists for pkgs commits built since the writer landed; the old pin predates it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…he index fetch-prebuilt.sh resolves kernel/rootfs/krun through the package map buildbot publishes per pkgs commit (arch -> package -> sha256) and pulls the content-addressed zstd tarballs directly — no toolchain, no graph evaluation, and no macOS shim. The justfile's artifacts recipe collapses to one OS-agnostic definition and libkrun fetches prebuilt too; the mip-based fetch scripts remain for the frozen CI lanes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@justfile`:
- Around line 327-340: Update the native minimald lifecycle checks in the
startup block and the related down path to validate the PID from "$pidf" before
accepting or signaling it. Confirm /proc/$pid/exe points to "{{minimald-bin}}"
and the process arguments reference "{{native-dir}}" as its state directory; if
validation fails, remove the stale PID file and socket, then start or stop
nothing based on that PID.
- Around line 343-346: Update the up-kvm recipe to use the configured,
caller-overridable VM boot timeout instead of the hard-coded 75-second value.
Reuse the existing timeout configuration defined near the top of the justfile,
while preserving the current minvmd run arguments and behavior.
In `@scripts/fetch-prebuilt.sh`:
- Around line 66-72: Update the package loop around the indexed sha256 lookup
and artifact download to validate that the digest is a well-formed SHA-256
value, then compute the downloaded "$TMP/$pkg.zst" file’s SHA-256 and require it
to match before running zstd or tar extraction. Reject malformed or mismatched
digests with an error and exit without installing the artifact.
- Around line 75-83: Update the kernel and rootfs branches of the WHAT case in
fetch-prebuilt.sh to copy each artifact to a temporary file in DEST’s directory,
then rename it to DEST only after install succeeds. Ensure the temporary path is
cleaned up on interruption or failure so artifacts never retain a partial file
and existing -f checks see only complete outputs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91af9923-f5ef-43d6-93cb-4dc400802a78
📒 Files selected for processing (9)
.minimal/minimal.tomlCONTRIBUTING.mdCross.tomldocs/ci-strategy.mdjustfilescripts/fetch-prebuilt.shscripts/minvmd-lifecycle.shscripts/reap-vms.shscripts/session-e2e.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/reap-vms.sh
- Cross.toml
- scripts/minvmd-lifecycle.sh
- scripts/session-e2e.sh
- CONTRIBUTING.md
…mald PID files survive reboots and PIDs get reused: up could adopt a foreign process as the running daemon and down could signal it. Validate /proc/<pid>/exe against this checkout's binary before trusting or signaling, clear stale socket/pidfile before a fresh spawn, and honor the caller-overridable READY window in up-kvm instead of a fixed 75s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The index sha256 is the content address — check the downloaded bytes against it (and reject malformed digests) before extraction, and rename file artifacts into place so an interrupted run can't leave a partial kernel/rootfs that the -f guards would trust forever. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Goals
Make the justfile the single local entry point for the promise in docs/ci-strategy.md §8 — the frozen CI workflows stay a thin scheduler, and everything they check can be built, tested, and executed locally:
artifacts/initramfs/gvproxy/minvmd-build/ … fetch and build the full stack on either host; recipes are[linux]/[macos]-dispatched like the lanes, sojust --listshows exactly what your machine can run.just ciis the PR gate set (fmt, clippy, cargo-deny, nextest, doctests, plus the#[ignore]surface no CI lane can run).test-vm,test-root-integration,e2e/e2e-native,test-lifecycle, andsoakmirror their CI lanes one-for-one; each recipe's comment names its counterpart.just upbrings up this host's default stack (macOS: Linux VM over Hypervisor.framework; Linux: host-native minimald),just up-kvmthe Linux VM mode,just downstops it. The DM1/2/3 nomenclature is retired.unamedispatch, top-levelexportreplaces per-recipe env stanzas, shared helpers replace copy-pasted preflights, and comments carry captions plus landmines only.En-route fixes: checkout-scoped
reap-vms.sh, overridable boot windows for slow/nested-KVM hosts, the installer's AppArmor remediation surfaced in the native lanes, and the e2e workdir pinned to the cache filesystem (hardlinks can't cross devices).Verification
After the final rewrite, on macOS:
just ci(493 tests),just e2e(full VM session proof incl. in-sandboxmin add), and a forced shim re-materialize — all green. Every Linux lane ran green earlier on a native Ubuntu/KVM host; the fixes that run surfaced are the last commits before the rewrite.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation