fix(sandbox): gate auto_detect noop assertion to windows only#245
Merged
emal-avala merged 1 commit intomainfrom Apr 24, 2026
Merged
fix(sandbox): gate auto_detect noop assertion to windows only#245emal-avala merged 1 commit intomainfrom
emal-avala merged 1 commit intomainfrom
Conversation
`auto_detect_off_macos_is_noop` used `#[cfg(not(target_os = "macos"))]` and asserted `auto_detect().name() == "noop"`. That assumption is false on any Linux box with `bwrap` on $PATH — auto_detect correctly returns `"bwrap"` there (see `make_bwrap_or_noop`). The companion test `auto_detect_on_linux_picks_bwrap_or_noop` already handles Linux correctly, so the non-macos test was redundantly covering Linux with a stricter-than-true contract. Narrow the gate to Windows, where no sandbox strategy is currently registered, so auto_detect genuinely returns noop. Rename to `auto_detect_on_windows_is_noop` to match the invariant. Add a brief comment explaining why the previous gate was wrong so this doesn't regress. Effect: `cargo test` now passes on any Linux dev machine regardless of whether bwrap is installed. CI on GitHub Actions ubuntu-latest (no bwrap) was already passing and continues to pass.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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
1 participant
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.
Summary
auto_detect_off_macos_is_noopwas gated on#[cfg(not(target_os = "macos"))]and assertedauto_detect().name() == "noop". That assumption is wrong on any Linux box withbwrapon$PATH—auto_detectcorrectly returns"bwrap"there (seemake_bwrap_or_noop). The companion testauto_detect_on_linux_picks_bwrap_or_noopalready handles Linux with the right predicate (bwrapORnoop), so the non-macos test was redundantly covering Linux with a stricter-than-true contract.Fix
Narrow the gate to Windows, where no sandbox strategy is registered, so
auto_detectgenuinely returnsnoop. Renamed toauto_detect_on_windows_is_noopto match the invariant, and added a comment so the reason for the narrower gate doesn't regress.Effect
cargo testnow passes on any Linux dev machine regardless of whetherbwrapis installed.ubuntu-latestrunners don't havebwrap, so CI was already passing and continues to pass.Test plan
cargo test -p agent-code-lib --lib sandbox::— all 51 pass locally (Linux with bwrap)cargo clippy --workspace --tests --no-deps -- -D warningscargo fmt --all --check