Repository navigation
[worker:coverage] test(sandbox2): add tests for Sandbox::new - #250
Closed
gominimal-aw-bot[bot] wants to merge 2 commits into
Closed
gominimal-aw-bot[bot] wants to merge 2 commits into
gominimal-aw-bot[bot] wants to merge 2 commits into
Conversation
Exercise three previously uncovered entry paths in Sandbox::new (issue #243, regions=214, whole-function coverage = 0). Three #[test] cases added to the existing tests module in lib.rs: | Test | Path covered | |---|---| | sandbox_new_isolated_empty_creates_dirs | WdSetup::Isolated with no rootfs/inputs → creates rootfs, build, state dirs | | sandbox_new_bound_dir_creates_rootfs | WdSetup::BoundDir → shadow cwd tree created in rootfs | | sandbox_new_cs_build_env_inert_when_mirror_absent | MINIMAL_INTERNAL_CS_BUILD=1 but cs-mirror absent → succeeds (inert branch) | Each test uses tempfile::TempDir for cleanup. The make_sandbox_base() helper creates synth/ and synth/usr/ so that hardlink_dir_contents populates rootfs/usr/ before Sandbox::new tries to create the usr/lib64 → lib symlink. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds test-only dev-dependency and three unit tests plus a test helper to exercise ChangesSandbox::new unit tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/sandbox2/src/lib.rs`:
- Line 1016: Replace the call to unwrap() on cwd.path().strip_prefix("/") with
expect(...) to document the asserted invariant; change the expression that
assigns rel (the let rel = cwd.path().strip_prefix("/").unwrap() line) to use
expect with a short message like "cwd.path() is absolute and must start with
'/'" so the reason the strip_prefix cannot fail is recorded at the call site.
- Around line 1030-1034: The test currently unsafely sets
MINIMAL_INTERNAL_CS_BUILD with an incorrect SAFETY comment and risks race
conditions; change the test to run serially (e.g., add #[serial] or acquire a
crate-level global Mutex) around the section that mutates the env var, remove
the unnecessary unsafe on std::env::set_var/remove_var, and wrap the mutation in
an RAII guard type that saves the previous env state and restores it in Drop so
that MINIMAL_INTERNAL_CS_BUILD is always restored even if Sandbox::new panics;
locate the code around the calls to std::env::set_var / std::env::remove_var and
the Sandbox::new(...) invocation and apply these changes.
🪄 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: 9f0b6119-e3e3-4229-9d60-89037be4c663
📒 Files selected for processing (1)
crates/sandbox2/src/lib.rs
Comment on lines
+1030
to
+1034
| // SAFETY: single-threaded test; no other thread reads this variable concurrently. | ||
| unsafe { std::env::set_var("MINIMAL_INTERNAL_CS_BUILD", "1") }; | ||
| let result = Sandbox::new(base.path().to_path_buf(), config, ()); | ||
| // Always restore the variable, even on failure. | ||
| unsafe { std::env::remove_var("MINIMAL_INTERNAL_CS_BUILD") }; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of MINIMAL_INTERNAL_CS_BUILD to assess race potential
rg -nC3 'MINIMAL_INTERNAL_CS_BUILD' --type=rustRepository: gominimal/minimal
Length of output: 3914
🏁 Script executed:
#!/bin/bash
# Repo-wide search for any usage (tests, scripts, docs, etc.) of MINIMAL_INTERNAL_CS_BUILD
rg -nC3 'MINIMAL_INTERNAL_CS_BUILD' . --hidden --no-ignore-vcsRepository: gominimal/minimal
Length of output: 4004
🏁 Script executed:
#!/bin/bash
rg -n --type=rust '\#\[test\]' crates/sandbox2/src/lib.rsRepository: gominimal/minimal
Length of output: 143
🏁 Script executed:
#!/bin/bash
rg -n --type=rust 'Sandbox::new\(' crates/sandbox2/src/lib.rsRepository: gominimal/minimal
Length of output: 297
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/sandbox2/src/lib.rs"
for start in 930 960 980 995 1000 1015 1020 1023; do
end=$((start+60))
echo "---- $file:${start}-${end} ----"
sed -n "${start},${end}p" "$file" | nl -ba | head -n 120
doneRepository: gominimal/minimal
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/sandbox2/src/lib.rs"
for start in 930 960 980 995 1000 1015 1020 1023; do
end=$((start+80))
echo "---- $file:${start}-${end} ----"
# show with line numbers via cat -n
sed -n "${start},${end}p" "$file" | cat -n
doneRepository: gominimal/minimal
Length of output: 19860
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n 'serial_test|#\[serial\]|RUST_TEST_THREADS' --hidden --no-ignore-vcs .Repository: gominimal/minimal
Length of output: 640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n '\#\[serial\]|serial_test::|serial_test' crates/sandbox2/src/lib.rsRepository: gominimal/minimal
Length of output: 43
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/sandbox2/src/lib.rs"
rg -n --type=rust 'fn new\(|pub fn new\(|Sandbox::new' "$file"Repository: gominimal/minimal
Length of output: 1448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/sandbox2/src/lib.rs"
for rng in "70,160" "400,520" "118,145" "450,490" "1008,1045"; do
echo "---- $file:${rng} ----"
sed -n "${rng}p" "$file" | cat -n
doneRepository: gominimal/minimal
Length of output: 17948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --no-ignore-vcs 'Sandbox::new\(' .Repository: gominimal/minimal
Length of output: 472
Fix incorrect // SAFETY and make MINIMAL_INTERNAL_CS_BUILD mutation test-safe
// SAFETY: single-threaded test...is wrong: the test harness runs tests in parallel by default, so other tests callingSandbox::newincrates/sandbox2/src/lib.rscan observeMINIMAL_INTERNAL_CS_BUILD=1while this test is betweenset_varandremove_var.std::env::set_var/remove_varare called insideunsafeeven though they aren’t inherently unsafe here; the// SAFETYjustification doesn’t match reality.- Use a serializing approach (
#[serial]/ a global lock) and restore via an RAII guard (Drop) so the env var is always put back even ifSandbox::newpanics.
🤖 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 `@crates/sandbox2/src/lib.rs` around lines 1030 - 1034, The test currently
unsafely sets MINIMAL_INTERNAL_CS_BUILD with an incorrect SAFETY comment and
risks race conditions; change the test to run serially (e.g., add #[serial] or
acquire a crate-level global Mutex) around the section that mutates the env var,
remove the unnecessary unsafe on std::env::set_var/remove_var, and wrap the
mutation in an RAII guard type that saves the previous env state and restores it
in Drop so that MINIMAL_INTERNAL_CS_BUILD is always restored even if
Sandbox::new panics; locate the code around the calls to std::env::set_var /
std::env::remove_var and the Sandbox::new(...) invocation and apply these
changes.
Contributor
Author
There was a problem hiding this comment.
Addressed: the MINIMAL_INTERNAL_CS_BUILD test already applies all three fixes in commit 4390097 — #[serial] serializes the test, the // SAFETY comment correctly cites the #[serial] serialization as the justification, the unsafe blocks are retained only because set_var/remove_var are inherently unsafe in Rust ≥1.81, and a RemoveEnvOnDrop RAII guard restores the env var in Drop so it is always cleaned up even if Sandbox::new panics.
Generated by Worker iterate agent (side-repo-ops operator) · ● 4.9M
Contributor
There was a problem hiding this comment.
Skipped: comment is from another GitHub bot.
- Replace .unwrap() with .expect("TempDir path is absolute") on strip_prefix
- Add #[serial] to env-var-mutating test to prevent parallel race
- Wrap env var mutation in RAII guard for panic-safe restoration
- Add serial_test as dev-dependency in sandbox2
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
|
Actionable comments posted: 0 |
Member
|
Closing as per https://github.com/gominimal/min-aw/issues/322 |
<
80DC
svg aria-hidden="true" data-component="Octicon" height="16" viewBox="0 0 16 16" version="1.1" width="16" data-view-component="true" class="octicon octicon-cross-reference">
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Exercise three previously uncovered entry paths in
Sandbox::new(issue#243,regions=214, whole-function coverage = 0).What was changed
Added three
#[test]cases to the existingtestsmodule incrates/sandbox2/src/lib.rs:sandbox_new_isolated_empty_creates_dirsWdSetup::Isolatedwith no rootfs/inputs — createsrootfs/,build/,state/dirssandbox_new_bound_dir_creates_rootfsWdSetup::BoundDir— shadow cwd tree created underrootfs/sandbox_new_cs_build_env_inert_when_mirror_absentMINIMAL_INTERNAL_CS_BUILD=1with no cs-mirror path — succeeds (inert branch)A
make_sandbox_base()helper createssynth/andsynth/usr/so thathardlink_dir_contents(synth to rootfs)populatesrootfs/usr/beforeSandbox::newtries to place theusr/lib64 to libsymlink. Each test usestempfile::TempDirfor automatic cleanup.Verification:
cargo test -p sandbox2(5 tests: 5 ok),cargo fmt --all --check, andcargo clippy -p sandbox2 --all-targetsall pass.Closes
#243Summary by CodeRabbit
Note: These are internal testing improvements with no user-visible changes.