Conversation
📦 BoxLite review — couldn't completepowered by BoxLite |
📝 WalkthroughWalkthroughThe PR adds static guest-tool builds with target-aware musl toolchains, verified dependency caches, ELF validation, CI packaging, and new Make targets. It also replaces recursive external ChangesGuest artifact build and validation
Descriptor-based ownership repair
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant build_guest
participant build_libseccomp
participant build_e2fsprogs
participant ArtifactUpload
GitHubActions->>build_guest: Build guest agent and tools
build_guest->>build_libseccomp: Acquire leased libseccomp
build_guest->>build_e2fsprogs: Build static guest tools
build_e2fsprogs->>build_guest: Return verified artifact paths
build_guest->>GitHubActions: Verify guest artifacts
GitHubActions->>ArtifactUpload: Package and upload Linux artifacts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
This PR extends the guest build pipeline to also produce and qualify static filesystem utilities (mke2fs, resize2fs) and hardens the guest’s ownership-repair path by replacing PATH-resolved chown -R with fd-relative syscalls. It fits into the guest artifact production/qualification flow by making guest outputs more deterministic (ELF shape checks, cache identity/publication contracts) and less dependent on host runtime tools.
Changes:
- Build and publish static musl
mke2fs/resize2fsalongside the guest agent, including deterministic output verification and atomic publication contracts. - Replace recursive external
chowninvocation with an fd-relative directory walk usingopenat/fstatat/fchownatwhile preserving the existing “sample first, best-effort warnings” policy. - Add CI + make targets to qualify these artifacts across Linux x64/arm64 and macOS cross-build structural validation.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/guest/src/storage/perms.rs |
Replaces PATH-based chown -R with fd-relative traversal + adds targeted unit/fixture tests for edge cases (symlinks, mount cycles, read errors). |
src/guest/Cargo.toml |
Enables nix dir feature needed for directory iteration APIs. |
scripts/util.sh |
Adds target→arch mapping and coherent musl toolchain resolution/export helpers. |
scripts/test/test-guest-tools.sh |
New end-to-end qualification script for guest-tools: ELF shape checks, cache/publication contract tests, and Linux runtime smoke. |
scripts/build/verify-guest-elf.sh |
New reusable verifier enforcing “static, non-PIE ET_EXEC, correct arch, no interp/dynamic/DT_NEEDED”. |
scripts/build/build-libseccomp.sh |
Refactors into a source-safe, generation/lease-based native cache for Linux headers + static libseccomp with verification and GC. |
scripts/build/build-guest.sh |
Builds guest tools + guest agent and validates all resulting guest artifacts via verify_guest_elf. |
scripts/build/build-e2fsprogs-guest.sh |
New build helper that snapshots source + headers, builds static tools, writes metadata, and publishes atomically. |
make/test.mk |
Adds test:guest-tools and test:guest-perms targets with Linux-only privileged execution flow. |
make/help.mk |
Documents new guest build/test entry points. |
make/build.mk |
Adds guest-tools target and threads PROFILE through make guest. |
.github/workflows/test.yml |
Adds a guest-tools CI job matrix and expands path filters/allowlists accordingly. |
.cargo/config.toml |
Updates musl target rustflags to enforce static, non-PIE linking consistent with ELF verification. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (12)
src/guest/src/storage/perms.rs (4)
898-902: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the short-path fixture parent independent of the default target layout.
Path::new(env!("CARGO_MANIFEST_DIR")).join("../../target")assumes the default target directory location. WithCARGO_TARGET_DIRset to another location, that directory may not exist andtempdir_infails. The intent, a short parent path for the Unix socket, stays valid if you create the directory first or readCARGO_TARGET_DIR.♻️ Proposed robust fixture parent
- let target_dir = Path::new(env!("CARGO_MANIFEST_DIR")).join("../../target"); + let target_dir = std::env::var_os("CARGO_TARGET_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| Path::new(env!("CARGO_MANIFEST_DIR")).join("../../target")); + fs::create_dir_all(&target_dir).expect("create short-path fixture parent");🤖 Prompt for 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. In `@src/guest/src/storage/perms.rs` around lines 898 - 902, Update the temporary-directory setup in the short-path fixture to avoid assuming the manifest-relative ../../target directory. Use the configured CARGO_TARGET_DIR when available, or create the selected parent directory before calling tempfile::Builder::tempdir_in, while preserving the short parent path needed for the Unix socket.
673-679: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not panic inside
MountGuard::drop.
umount2(...).expect(...)panics if the unmount fails. The guard drops during unwinding when a test assertion already failed, and a panic during unwinding aborts the process. That hides the original assertion message. Report the failure without panicking.♻️ Proposed non-panicking guard
impl Drop for MountGuard { fn drop(&mut self) { - umount2(&self.0, MntFlags::MNT_DETACH).expect("unmount test bind mount"); + if let Err(error) = umount2(&self.0, MntFlags::MNT_DETACH) { + eprintln!("failed to unmount test bind mount {}: {error}", self.0.display()); + } } }🤖 Prompt for 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. In `@src/guest/src/storage/perms.rs` around lines 673 - 679, Update MountGuard::drop to remove the expect-based panic and report umount2 failures through the test’s established non-panicking failure mechanism. Preserve cleanup during unwinding and ensure an unmount error never triggers a second panic or masks the original assertion failure.
269-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute the fault injection through one seam instead of duplicating each syscall call site.
Four call sites now exist twice: once under
#[cfg(test)]with the injected error and once under#[cfg(not(test))]with the real syscall. An edit to a production branch does not fail any test if the test branch keeps the old form, so the tested path and the shipped path can diverge silently in a security-sensitive traversal.Wrap each syscall in a small private method that performs the injection check first and then calls the real syscall once. Example shape:
♻️ Proposed single-call-site seam
impl RecursiveChowner { fn open_child(&mut self, parent_fd: i32, name: &std::ffi::CStr, path: &Path) -> nix::Result<i32> { #[cfg(test)] if self.test_faults.should_fail_descent(path, TestDescentFailure::Open) { return Err(nix::errno::Errno::EMFILE); } openat( Some(parent_fd), name, OFlag::O_RDONLY | OFlag::O_DIRECTORY | OFlag::O_NOFOLLOW | OFlag::O_CLOEXEC, Mode::empty(), ) } }Also applies to: 346-367, 376-387, 410-422
🤖 Prompt for 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. In `@src/guest/src/storage/perms.rs` around lines 269 - 288, Consolidate the duplicated test and production syscall branches in RecursiveChowner by routing each affected operation, including the next-entry retrieval near stack traversal and the other noted call sites, through a private method seam. Have each method perform its #[cfg(test)] fault-injection check first, then invoke the real syscall exactly once, while preserving existing error behavior and production semantics.
333-344: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSkip ownership syscalls for matching owners
Before each
fchownatandfchown, compare the correspondingstat.st_uidandstat.st_gidwith the target IDs. Linux can clearS_ISUIDandS_ISGIDeven when the requested owner already matches. Matching entries on read-only filesystems can also produce unnecessaryEROFSfailures.Apply this to the fallback paths and deferred directory chowns. Carry the owner state in
DirectoryFrame. Update thechangedand failure-count assertions, and add coverage for setuid preservation.🤖 Prompt for 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. In `@src/guest/src/storage/perms.rs` around lines 333 - 344, Update the ownership-changing flow around fstatat and chown_entry to compare stat.st_uid and stat.st_gid with the target IDs before every fchownat or fchown call, including fallback and deferred directory paths. Carry the relevant owner state through DirectoryFrame, skip matching-owner syscalls, and update changed/failure-count assertions plus coverage verifying setuid preservation.scripts/build/build-libseccomp.sh (3)
1351-1351: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
$_NATIVE_CACHE_SELECTION_MAX_ATTEMPTSinstead of the literal3.
with_linux_headers_for_archuses the constant at Line 906. This retry loop uses a magic number, so the two facades can drift.♻️ Proposed change
- while [ "$attempt" -le 3 ]; do + while [ "$attempt" -le "$_NATIVE_CACHE_SELECTION_MAX_ATTEMPTS" ]; do🤖 Prompt for 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. In `@scripts/build/build-libseccomp.sh` at line 1351, Update the retry condition in the loop around with_linux_headers_for_arch to compare attempt against $_NATIVE_CACHE_SELECTION_MAX_ATTEMPTS instead of the literal 3, matching the existing constant usage and keeping both retry facades synchronized.
476-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_native_cache_find_free_fdand_guest_tools_find_free_fdare the same helper copied into two scripts. Both bodies scan descriptors 10 through 99 with the identicaleval ": <&N"/eval ": >&N"probe and return the first free number. Both files already source or can sourcescripts/util.sh, so the helper belongs there once.
scripts/build/build-libseccomp.sh#L476-L487: delete_native_cache_find_free_fdand call the shared helper from_native_cache_with_generation_leaseat Line 296.scripts/build/build-e2fsprogs-guest.sh#L539-L550: delete_guest_tools_find_free_fdand call the shared helper from_guest_tools_acquire_publish_lockat Lines 597 and 607.Add the single implementation to
scripts/util.shnext to the other toolchain helpers.🤖 Prompt for 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. In `@scripts/build/build-libseccomp.sh` around lines 476 - 487, Move the duplicated descriptor-scanning helper into scripts/util.sh beside the other toolchain helpers. In scripts/build/build-libseccomp.sh lines 476-487, remove _native_cache_find_free_fd and update _native_cache_with_generation_lease to call the shared helper; in scripts/build/build-e2fsprogs-guest.sh lines 539-550, remove _guest_tools_find_free_fd and update _guest_tools_acquire_publish_lock at both call sites to use it. Preserve scanning descriptors 10 through 99 and returning the first available descriptor.
1033-1034: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe cache identity hardcodes
cflagsand theconfigureline instead of deriving them from the build.Lines 1033-1034 write literal strings.
_build_libseccomp_generationbuilds withCFLAGS="-Os -fPIC"at Line 1164 and the configure flags at Lines 1155-1166. The two lists are separate copies of the same facts.If someone changes the configure invocation or the CFLAGS without editing this identity function, the cache key stays the same. Stale generations then satisfy
_native_cache_libseccomp_matchand the new build flags never take effect.Define the flag list once and reference it from both places.
🤖 Prompt for 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. In `@scripts/build/build-libseccomp.sh` around lines 1033 - 1034, Update the cache identity function and _build_libseccomp_generation to share one source of truth for the CFLAGS and configure flags. Define the flag values once, reuse them when generating the identity output and when invoking the build, and remove the duplicated literals so changes to either build configuration invalidate the cache.scripts/test/test-guest-tools.sh (1)
741-741: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove this no-op line.
At Line 741 no
mvfunction is defined yet, sodeclare -f mvfails, the command substitution is empty, andeval ""does nothing. The saved copymv_before_holder_lifetime_testis never created and never referenced. Line 766 already restores the builtin withunset -f mv.♻️ Proposed change
- eval "$(declare -f mv | sed '1s/mv/mv_before_holder_lifetime_test/')" 2>/dev/null || true mv() {🤖 Prompt for 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. In `@scripts/test/test-guest-tools.sh` at line 741, Remove the no-op eval command that attempts to create mv_before_holder_lifetime_test; no replacement is needed because the function is not defined or used, and the existing unset -f mv restoration remains unchanged.scripts/build/build-e2fsprogs-guest.sh (2)
947-967: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
makeis not part of the guest-tools cache signature.Line 964-966 invoke
makethroughPATH. The signature material at Lines 902-934 recordscc,build_cc,ar,ranlib,stripand each of their--versionlines, but notmake.
scripts/build/build-libseccomp.shdoes recordmake-pathandmake-versionin its identity at Lines 1031-1032, andscripts/test/test-guest-native-cache.shasserts that behavior at Line 1682. The guest-tools cache is inconsistent with that contract: a differentmakeproduces a cache hit on the old artifacts.Resolve
makewithcommand -vand add its path and version line tosignature_material.🤖 Prompt for 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. In `@scripts/build/build-e2fsprogs-guest.sh` around lines 947 - 967, Update the guest-tools signature construction near the existing compiler and binutils identity data to resolve the make executable with command -v, capture its version line, and append both make-path and make-version entries to signature_material. Keep the existing make invocations in the configure/build flow unchanged.
937-940: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffThe cache-hit check runs after the two most expensive steps.
Lines 810-812 copy the whole e2fsprogs working tree into a private snapshot and hash it. Lines 822-825 copy the Linux header generation and hash it. Only then does Line 937 test the verified-output cache.
build-guest.shcallsbuild_guest_toolson everymake guestinvocation, so a fully cached tree still pays a full e2fsprogs tree copy plus two SHA-256 tree walks each time.The four cheap fingerprints already computed at Lines 789-808 (
source_commit,source_status,source_diff_sha,source_untracked_sha) identify the same source state. Consider computing a preliminary signature from those, testing the output cache first, and creating the snapshots only on a miss. Keepsource_snapshot_idin the published metadata so the immutability contract thatscripts/test/test-guest-tools.shasserts still holds.🤖 Prompt for 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. In `@scripts/build/build-e2fsprogs-guest.sh` around lines 937 - 940, The cache check in build_guest_tools currently occurs only after expensive source and header snapshot/hash operations. Use the existing source_commit, source_status, source_diff_sha, and source_untracked_sha values to compute a preliminary cache signature and call _guest_tools_verify_output before creating snapshots; on a cache miss, perform the existing snapshot work and final verification. Continue publishing source_snapshot_id in the metadata so the immutability contract remains intact.scripts/test/test-guest-native-cache.sh (2)
71-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
define_toolchain_stubsis never called, and its body is duplicated in eleven places.Shellcheck flags Lines 72-80 as uninvoked. Every consumer inlines the same
target_to_arch/resolve_musl_cc/resolve_musl_toolstubs inside itsbash -cpayload, for example Lines 116-120, 403-407, 487-491, 581-585, 669-673, 748-752, 819-823, 922-926, 1178-1182, and 1503-1507.The stubs must be defined inside the child shell, so a plain function call cannot replace them. Define the stub text once as a string and interpolate it, as Line 1391 already does with
common_script. Then delete this function.🤖 Prompt for 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. In `@scripts/test/test-guest-native-cache.sh` around lines 71 - 81, Replace the unused define_toolchain_stubs function with a single reusable string containing the target_to_arch, resolve_musl_cc, and resolve_musl_tool definitions, following the existing common_script pattern. Interpolate that string into each bash -c payload currently duplicating these stubs, including the listed consumers, and then remove define_toolchain_stubs.
259-259: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the synchronization timeout configurable instead of a fixed 10 seconds.
Lines 259, 617, 701, 836, 1253, and 1422 hardcode
-t 10. The sibling suitescripts/test/test-guest-tools.shreadsBOXLITE_GUEST_TOOLS_TEST_SYNC_TIMEOUT_SECONDSat Line 99 and defaults to 300.The wait at Line 1253 covers a full
ensure_linux_headers_for_archrun, including download-wrapper copy and tar extraction. A loaded CI runner can exceed 10 seconds and produce a flaky failure. Bothmake test:guest-toolssteps run in the same CI job, so the two suites should use the same budget.Also applies to: 1253-1253
🤖 Prompt for 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. In `@scripts/test/test-guest-native-cache.sh` at line 259, Replace the hardcoded 10-second timeouts on all listed `read -t` synchronization waits with a configurable timeout variable, reading `BOXLITE_GUEST_TOOLS_TEST_SYNC_TIMEOUT_SECONDS` and defaulting to 300 seconds as in `test-guest-tools.sh`. Reuse that variable at the waits around lines 259, 617, 701, 836, 1253, and 1422, including the `ensure_linux_headers_for_arch` flow.
🤖 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 @.github/workflows/test.yml:
- Around line 448-450: Update the “Install native guest test dependencies” step
to run sudo apt-get update immediately before installing jq and libseccomp-dev,
preserving the Linux-only condition and existing package installation behavior.
In `@make/test.mk`:
- Around line 265-276: Update the user-namespace branch in the guest-permissions
test flow so it does not run the two narrowed privileged suites before an
unconditional failure. Choose and implement one explicit policy: either return
success after a clear warning that sampling and blanket ownership coverage was
skipped, or fail immediately before invoking those suites when full coverage is
mandatory. Keep the existing unshare detection and root/passwordless-sudo paths
unchanged.
In `@scripts/build/build-guest.sh`:
- Around line 58-64: Initialize GUEST_TARGET before check_prerequisites invokes
init_musl_toolchain, including when the script is run without a preexisting
GUEST_TARGET. Update the build-guest initialization flow to call the existing
guest-variable setup before check_prerequisites, while preserving the current
SCRIPT_BUILD_DIR and SCRIPT_DIR initialization.
In `@scripts/util.sh`:
- Around line 165-175: Update init_guest_vars so the target_to_arch call
propagates a nonzero status when GUEST_TARGET is set, causing initialization to
fail immediately instead of assigning an empty GUEST_ARCH and returning success;
preserve the existing host-detection path unchanged.
---
Nitpick comments:
In `@scripts/build/build-e2fsprogs-guest.sh`:
- Around line 947-967: Update the guest-tools signature construction near the
existing compiler and binutils identity data to resolve the make executable with
command -v, capture its version line, and append both make-path and make-version
entries to signature_material. Keep the existing make invocations in the
configure/build flow unchanged.
- Around line 937-940: The cache check in build_guest_tools currently occurs
only after expensive source and header snapshot/hash operations. Use the
existing source_commit, source_status, source_diff_sha, and source_untracked_sha
values to compute a preliminary cache signature and call
_guest_tools_verify_output before creating snapshots; on a cache miss, perform
the existing snapshot work and final verification. Continue publishing
source_snapshot_id in the metadata so the immutability contract remains intact.
In `@scripts/build/build-libseccomp.sh`:
- Line 1351: Update the retry condition in the loop around
with_linux_headers_for_arch to compare attempt against
$_NATIVE_CACHE_SELECTION_MAX_ATTEMPTS instead of the literal 3, matching the
existing constant usage and keeping both retry facades synchronized.
- Around line 476-487: Move the duplicated descriptor-scanning helper into
scripts/util.sh beside the other toolchain helpers. In
scripts/build/build-libseccomp.sh lines 476-487, remove
_native_cache_find_free_fd and update _native_cache_with_generation_lease to
call the shared helper; in scripts/build/build-e2fsprogs-guest.sh lines 539-550,
remove _guest_tools_find_free_fd and update _guest_tools_acquire_publish_lock at
both call sites to use it. Preserve scanning descriptors 10 through 99 and
returning the first available descriptor.
- Around line 1033-1034: Update the cache identity function and
_build_libseccomp_generation to share one source of truth for the CFLAGS and
configure flags. Define the flag values once, reuse them when generating the
identity output and when invoking the build, and remove the duplicated literals
so changes to either build configuration invalidate the cache.
In `@scripts/test/test-guest-native-cache.sh`:
- Around line 71-81: Replace the unused define_toolchain_stubs function with a
single reusable string containing the target_to_arch, resolve_musl_cc, and
resolve_musl_tool definitions, following the existing common_script pattern.
Interpolate that string into each bash -c payload currently duplicating these
stubs, including the listed consumers, and then remove define_toolchain_stubs.
- Line 259: Replace the hardcoded 10-second timeouts on all listed `read -t`
synchronization waits with a configurable timeout variable, reading
`BOXLITE_GUEST_TOOLS_TEST_SYNC_TIMEOUT_SECONDS` and defaulting to 300 seconds as
in `test-guest-tools.sh`. Reuse that variable at the waits around lines 259,
617, 701, 836, 1253, and 1422, including the `ensure_linux_headers_for_arch`
flow.
In `@scripts/test/test-guest-tools.sh`:
- Line 741: Remove the no-op eval command that attempts to create
mv_before_holder_lifetime_test; no replacement is needed because the function is
not defined or used, and the existing unset -f mv restoration remains unchanged.
In `@src/guest/src/storage/perms.rs`:
- Around line 898-902: Update the temporary-directory setup in the short-path
fixture to avoid assuming the manifest-relative ../../target directory. Use the
configured CARGO_TARGET_DIR when available, or create the selected parent
directory before calling tempfile::Builder::tempdir_in, while preserving the
short parent path needed for the Unix socket.
- Around line 673-679: Update MountGuard::drop to remove the expect-based panic
and report umount2 failures through the test’s established non-panicking failure
mechanism. Preserve cleanup during unwinding and ensure an unmount error never
triggers a second panic or masks the original assertion failure.
- Around line 269-288: Consolidate the duplicated test and production syscall
branches in RecursiveChowner by routing each affected operation, including the
next-entry retrieval near stack traversal and the other noted call sites,
through a private method seam. Have each method perform its #[cfg(test)]
fault-injection check first, then invoke the real syscall exactly once, while
preserving existing error behavior and production semantics.
- Around line 333-344: Update the ownership-changing flow around fstatat and
chown_entry to compare stat.st_uid and stat.st_gid with the target IDs before
every fchownat or fchown call, including fallback and deferred directory paths.
Carry the relevant owner state through DirectoryFrame, skip matching-owner
syscalls, and update changed/failure-count assertions plus coverage verifying
setuid preservation.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b3e15b3f-d139-40d7-bdf8-ae1fd900899b
📒 Files selected for processing (14)
.cargo/config.toml.github/workflows/test.ymlmake/build.mkmake/help.mkmake/test.mkscripts/build/build-e2fsprogs-guest.shscripts/build/build-guest.shscripts/build/build-libseccomp.shscripts/build/verify-guest-elf.shscripts/test/test-guest-native-cache.shscripts/test/test-guest-tools.shscripts/util.shsrc/guest/Cargo.tomlsrc/guest/src/storage/perms.rs
| elif unshare --user --map-root-user --mount --propagation private -- \ | ||
| true >/dev/null 2>&1; then \ | ||
| echo "⚠️ No passwordless sudo; running namespace-compatible ownership tests only"; \ | ||
| unshare --user --map-root-user --mount --propagation private -- \ | ||
| "$$test_binary" \ | ||
| storage::perms::tests::privileged_fix_does_not_depend_on_path \ | ||
| --exact --ignored --test-threads=1 || exit $$?; \ | ||
| unshare --user --map-root-user --mount --propagation private -- \ | ||
| "$$test_binary" 'storage::perms::tests::privileged_c' \ | ||
| --ignored --test-threads=1 || exit $$?; \ | ||
| echo "❌ Full sampling/blanket ownership tests require real root or passwordless sudo"; \ | ||
| exit 1; \ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
The user-namespace branch always fails, so the two test runs before it are wasted.
Lines 268-274 run two narrowed privileged suites. Line 275 then prints an error and Line 276 exits 1, regardless of those results.
On a developer machine with user namespaces but without root and without passwordless sudo, make test:guest-perms can never succeed. It compiles the guest crate, runs the unprivileged suite, runs two more namespaced suites, and then reports failure. make/help.mk Line 45 describes the target as "privileged cases use sudo", which does not signal a guaranteed failure.
Choose one behavior and make it explicit:
- If partial coverage is acceptable locally, exit 0 and print one clear warning that the sampling and blanket cases were skipped.
- If full coverage is mandatory, fail before running the two partial suites so the developer is not made to wait.
♻️ Proposed change for the fail-fast option
elif unshare --user --map-root-user --mount --propagation private -- \
true >/dev/null 2>&1; then \
- echo "⚠️ No passwordless sudo; running namespace-compatible ownership tests only"; \
- unshare --user --map-root-user --mount --propagation private -- \
- "$$test_binary" \
- storage::perms::tests::privileged_fix_does_not_depend_on_path \
- --exact --ignored --test-threads=1 || exit $$?; \
- unshare --user --map-root-user --mount --propagation private -- \
- "$$test_binary" 'storage::perms::tests::privileged_c' \
- --ignored --test-threads=1 || exit $$?; \
echo "❌ Full sampling/blanket ownership tests require real root or passwordless sudo"; \
+ echo " User namespaces alone cannot cover the sampling and blanket cases."; \
exit 1; \
else \🤖 Prompt for 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.
In `@make/test.mk` around lines 265 - 276, Update the user-namespace branch in the
guest-permissions test flow so it does not run the two narrowed privileged
suites before an unconditional failure. Choose and implement one explicit
policy: either return success after a clear warning that sampling and blanket
ownership coverage was skipped, or fail immediately before invoking those suites
when full coverage is mandatory. Keep the existing unshare detection and
root/passwordless-sudo paths unchanged.
| check_prerequisites() { | ||
| print_section "Checking prerequisites..." | ||
| require_command "rustc" "Run: scripts/setup/setup-macos.sh (or setup-ubuntu.sh)" | ||
| require_musl | ||
| init_musl_toolchain "$GUEST_TARGET" | ||
| print_success "All prerequisites satisfied" | ||
| echo "" | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm GUEST_TARGET initialization and SCRIPT_BUILD_DIR definition in build-guest.sh.
set -euo pipefail
echo "=== top-of-file setup (lines 1-60) ==="
sed -n '1,60p' scripts/build/build-guest.sh
echo "=== init_guest_vars call sites ==="
rg -n '\binit_guest_vars\b' scripts/ || echo "no init_guest_vars call found"
echo "=== SCRIPT_BUILD_DIR assignments ==="
rg -n 'SCRIPT_BUILD_DIR\s*=' scripts/ || echo "SCRIPT_BUILD_DIR never assigned"Repository: boxlite-ai/boxlite
Length of output: 2537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== build-guest.sh control flow and references ==="
sed -n '55,135p' scripts/build/build-guest.sh
echo "=== util.sh guest-variable initialization ==="
sed -n '145,225p' scripts/util.sh
echo "=== init_musl_toolchain definition and callers ==="
rg -n -A35 -B8 'init_musl_toolchain' scriptsRepository: boxlite-ai/boxlite
Length of output: 10973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== target/tool resolution ==="
sed -n '70,150p' scripts/util.sh
echo "=== all GUEST_TARGET assignments and initialization calls ==="
rg -n '\b(GUEST_TARGET|init_guest_vars)\b' scripts .github/workflowsRepository: boxlite-ai/boxlite
Length of output: 9013
Initialize GUEST_TARGET before check_prerequisites runs. When build-guest.sh runs without GUEST_TARGET, sourcing util.sh does not call init_guest_vars; init_musl_toolchain receives an empty target and exits before the build. SCRIPT_BUILD_DIR and SCRIPT_DIR are both defined correctly.
🤖 Prompt for 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.
In `@scripts/build/build-guest.sh` around lines 58 - 64, Initialize GUEST_TARGET
before check_prerequisites invokes init_musl_toolchain, including when the
script is run without a preexisting GUEST_TARGET. Update the build-guest
initialization flow to call the existing guest-variable setup before
check_prerequisites, while preserving the current SCRIPT_BUILD_DIR and
SCRIPT_DIR initialization.
| # Initialize guest target and arch variables | ||
| init_guest_vars() { | ||
| local arch=$(detect_host_arch) | ||
| GUEST_TARGET=$(map_arch_to_target "$arch") | ||
| GUEST_ARCH=$(normalize_arch "$arch") | ||
| local arch | ||
|
|
||
| if [ -n "${GUEST_TARGET:-}" ]; then | ||
| GUEST_ARCH=$(target_to_arch "$GUEST_TARGET") | ||
| else | ||
| arch=$(detect_host_arch) | ||
| GUEST_TARGET=$(map_arch_to_target "$arch") | ||
| GUEST_ARCH=$(normalize_arch "$arch") | ||
| fi |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Propagate target_to_arch failure in init_guest_vars.
If GUEST_TARGET holds an unsupported triple, target_to_arch prints an error and returns 1, but init_guest_vars ignores that status. GUEST_ARCH becomes empty, the function exports it, and returns 0. Callers continue with an invalid toolchain configuration until a later helper fails.
.github/workflows/test.yml sets GUEST_TARGET from the job matrix, so this branch is reachable in CI.
🛡️ Proposed fix to fail fast
if [ -n "${GUEST_TARGET:-}" ]; then
- GUEST_ARCH=$(target_to_arch "$GUEST_TARGET")
+ GUEST_ARCH=$(target_to_arch "$GUEST_TARGET") || return 1
else
arch=$(detect_host_arch)
- GUEST_TARGET=$(map_arch_to_target "$arch")
- GUEST_ARCH=$(normalize_arch "$arch")
+ GUEST_TARGET=$(map_arch_to_target "$arch") || return 1
+ GUEST_ARCH=$(normalize_arch "$arch") || return 1
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Initialize guest target and arch variables | |
| init_guest_vars() { | |
| local arch=$(detect_host_arch) | |
| GUEST_TARGET=$(map_arch_to_target "$arch") | |
| GUEST_ARCH=$(normalize_arch "$arch") | |
| local arch | |
| if [ -n "${GUEST_TARGET:-}" ]; then | |
| GUEST_ARCH=$(target_to_arch "$GUEST_TARGET") | |
| else | |
| arch=$(detect_host_arch) | |
| GUEST_TARGET=$(map_arch_to_target "$arch") | |
| GUEST_ARCH=$(normalize_arch "$arch") | |
| fi | |
| # Initialize guest target and arch variables | |
| init_guest_vars() { | |
| local arch | |
| if [ -n "${GUEST_TARGET:-}" ]; then | |
| GUEST_ARCH=$(target_to_arch "$GUEST_TARGET") || return 1 | |
| else | |
| arch=$(detect_host_arch) | |
| GUEST_TARGET=$(map_arch_to_target "$arch") || return 1 | |
| GUEST_ARCH=$(normalize_arch "$arch") || return 1 | |
| fi |
🤖 Prompt for 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.
In `@scripts/util.sh` around lines 165 - 175, Update init_guest_vars so the
target_to_arch call propagates a nonzero status when GUEST_TARGET is set,
causing initialization to fail immediately instead of assigning an empty
GUEST_ARCH and returning success; preserve the existing host-detection path
unchanged.
c1af2cc to
d393531
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/build/verify-guest-elf.sh:18
verify_guest_elfexportsLC_ALL=C, which mutates the caller’s environment when this file is sourced (and persists after the function returns). That contradicts the “safe to source” contract and can change downstream tooling behavior in the same shell (e.g., locale-sensitivesort,awk,grep). Prefer running the locale override on the specificreadelf/grepinvocations (e.g.,LC_ALL=C readelf …) or restoring the previousLC_ALLon every return path.
local LC_ALL=C
export LC_ALL
Build and validate static mke2fs and resize2fs alongside the guest agent. Replace the external chown command with fd-relative syscall traversal, and add cache lifecycle, qualification, and CI coverage.
d393531 to
70291a1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (1)
make/test.mk:253
- The
cargo test --no-run | jq | tailpipeline can fail without failing the Make target (because the recipe doesn’t enablepipefail, so acargo/jqerror can be masked bytail). This can make the privileged test selection proceed with an empty/incorrecttest_binaryand hide the real failure cause.
test_binary=$$(cargo test -p boxlite-guest --bin boxlite-guest --no-run \
--message-format=json | jq -r \
'select(.reason == "compiler-artifact" and .profile.test == true and .target.name == "boxlite-guest" and .executable != null) | .executable' | tail -1); \
There was a problem hiding this comment.
🧹 Nitpick comments (2)
scripts/test/test-guest-tools.sh (2)
945-949: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
chmodpair.Line 947 applies
0644to every file in both directories. That overwrites the0755modes set at lines 945-946. Lines 948-949 then re-apply0755. Lines 945-946 have no effect.♻️ Proposed cleanup
- chmod 0755 "$publish_output/mke2fs" "$publish_output/resize2fs" - chmod 0755 "$publish_staging/mke2fs" "$publish_staging/resize2fs" chmod 0644 "$publish_output"/* "$publish_staging"/* chmod 0755 "$publish_output/mke2fs" "$publish_output/resize2fs" chmod 0755 "$publish_staging/mke2fs" "$publish_staging/resize2fs"🤖 Prompt for 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. In `@scripts/test/test-guest-tools.sh` around lines 945 - 949, Remove the initial redundant chmod commands targeting mke2fs and resize2fs before the 0644 wildcard chmod; retain the final 0755 commands so those executables end with the intended permissions.
741-741: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mv_before_holder_lifetime_testis never used.Line 741 saves a copy of any existing
mvfunction under a new name. No code calls that copy. Line 766 usesunset -f mv, which restores the built-inmvand not the saved function. Remove line 741, or restore the saved function at line 766.🤖 Prompt for 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. In `@scripts/test/test-guest-tools.sh` at line 741, Remove the unused mv_before_holder_lifetime_test function copy created before the holder lifetime test, or update the cleanup around unset -f mv to restore that saved function instead of the built-in mv; ensure the test’s original mv behavior is preserved.
🤖 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.
Nitpick comments:
In `@scripts/test/test-guest-tools.sh`:
- Around line 945-949: Remove the initial redundant chmod commands targeting
mke2fs and resize2fs before the 0644 wildcard chmod; retain the final 0755
commands so those executables end with the intended permissions.
- Line 741: Remove the unused mv_before_holder_lifetime_test function copy
created before the holder lifetime test, or update the cleanup around unset -f
mv to restore that saved function instead of the built-in mv; ensure the test’s
original mv behavior is preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9fd2ccf2-1f8b-4da6-8f07-217c56808ea4
📒 Files selected for processing (2)
scripts/build/verify-guest-elf.shscripts/test/test-guest-tools.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/build/verify-guest-elf.sh
|
|
||
| use std::path::Path; | ||
| use std::process::Command; | ||
| use std::os::fd::{AsRawFd, FromRawFd, OwnedFd}; |
There was a problem hiding this comment.
Extract this as one or multiple separate PRs. It's too large to review
There was a problem hiding this comment.
ok, i will close this pr and use multi pr to merge.
| @@ -0,0 +1,132 @@ | |||
| #!/bin/bash | |||
Summary
Build and qualify static
mke2fsandresize2fsbinaries alongside the guest agent, and replace the guest's PATH-based recursivechownwith fd-relative syscalls. The change adds deterministic cache/publication handling and cross-platform qualification without injecting the tools into rootfs or runtime packages.Call graph
Before
guest (Make target · make/build.mk:5) — builds only the guest agent
└─ build_guest_binary (script · scripts/build/build-guest.sh:92) — invokes the guest Cargo build
BlockDeviceMount::mount (Guest · src/guest/src/storage/block_device.rs:25) — mounts the container rootfs
└─ OwnershipFixer::fix_if_needed (Guest · src/guest/src/storage/perms.rs:25) — samples ownership
└─ external
chown -R(old OwnershipFixer · src/guest/src/storage/perms.rs) — resolves through PATHAfter
guest (Make target · make/build.mk:5) — builds and validates all guest artifacts
├─ build_guest_tools (script · scripts/build/build-guest.sh:99)
│ └─ ensure_guest_e2fsprogs_for_target (script · scripts/build/build-e2fsprogs-guest.sh:721)
│ └─ verify_guest_elf (script · scripts/build/verify-guest-elf.sh:10)
└─ build_guest_binary (script · scripts/build/build-guest.sh:92)
└─ verify_guest_elf (script · scripts/build/verify-guest-elf.sh:10)
BlockDeviceMount::mount (Guest · src/guest/src/storage/block_device.rs:25) — mounts the container rootfs
└─ OwnershipFixer::fix_if_needed (Guest · src/guest/src/storage/perms.rs:25) — preserves sampling policy
├─ OwnershipRoot::open (Guest · src/guest/src/storage/perms.rs:95) — opens every path component by fd
└─ RecursiveChowner::chown (Guest · src/guest/src/storage/perms.rs:264) — performs bounded fd-relative DFS
Changes
ET_EXEC, and absence of interpreter, dynamic segments, and needed libraries.chowncommand with component-relativeopenat/fstatat/fchownat/fchowntraversal while preserving sampling and best-effort warning behavior.How to verify
make guest PROFILE=releasemake test:guest-tools PROFILE=releasemake test:guest-tools PROFILE=debugmake test:guest-permsmake test:unit:guestRisks / rollout
Summary by CodeRabbit
New Features
Bug Fixes