From ec089fd22aba2c8df074b8abfd42d286b38b399c Mon Sep 17 00:00:00 2001 From: viyatb-oai Date: Fri, 27 Mar 2026 08:41:06 -0700 Subject: [PATCH] fix(sandbox): fix bwrap lookup for multi-entry PATH (#15973) ## Summary - split the joined `PATH` before running system `bwrap` lookup - keep the existing workspace-local `bwrap` skip behavior intact - add regression tests that exercise real multi-entry search paths ## Why The PATH-based lookup added in #15791 still wrapped the raw `PATH` environment value as a single `PathBuf` before passing it through `join_paths()`. On Unix, a normal multi-entry `PATH` contains `:`, so that wrapper path is invalid as one path element and the lookup returns `None`. That made Codex behave as if no system `bwrap` was installed even when `bwrap` was available on `PATH`, which is what users in #15340 were still hitting on `0.117.0-alpha.25`. ## Impact System `bwrap` discovery now works with normal multi-entry `PATH` values instead of silently falling back to the vendored binary. Fixes #15340. ## Validation - `just fmt` - `cargo test -p codex-sandboxing` - `cargo test -p codex-linux-sandbox` - `just fix -p codex-sandboxing` - `just argument-comment-lint` --- codex-rs/core/tests/suite/approvals.rs | 2 +- codex-rs/sandboxing/src/bwrap.rs | 2 +- codex-rs/sandboxing/src/bwrap_tests.rs | 10 ++++++---- 3 files changed, 8 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs index 6ec05bedc..1a7183a68 100644 --- a/codex-rs/core/tests/suite/approvals.rs +++ b/codex-rs/core/tests/suite/approvals.rs @@ -179,7 +179,7 @@ impl ActionKind { Ok((event, Some(command))) } ActionKind::RunCommand { command } => { - let event = shell_event(call_id, command, 1_000, sandbox_permissions)?; + let event = shell_event(call_id, command, 2_000, sandbox_permissions)?; Ok((event, Some(command.to_string()))) } ActionKind::RunUnifiedExecCommand { diff --git a/codex-rs/sandboxing/src/bwrap.rs b/codex-rs/sandboxing/src/bwrap.rs index 00cf55f7f..2ce394989 100644 --- a/codex-rs/sandboxing/src/bwrap.rs +++ b/codex-rs/sandboxing/src/bwrap.rs @@ -20,7 +20,7 @@ fn system_bwrap_warning_for_lookup(system_bwrap_path: Option) -> Option pub fn find_system_bwrap_in_path() -> Option { let search_path = std::env::var_os("PATH")?; let cwd = std::env::current_dir().ok()?; - find_system_bwrap_in_search_paths(std::iter::once(PathBuf::from(search_path)), &cwd) + find_system_bwrap_in_search_paths(std::env::split_paths(&search_path), &cwd) } fn find_system_bwrap_in_search_paths( diff --git a/codex-rs/sandboxing/src/bwrap_tests.rs b/codex-rs/sandboxing/src/bwrap_tests.rs index e379de3fb..8aa9a8020 100644 --- a/codex-rs/sandboxing/src/bwrap_tests.rs +++ b/codex-rs/sandboxing/src/bwrap_tests.rs @@ -33,7 +33,7 @@ exit 1 } #[test] -fn finds_first_executable_bwrap_in_search_paths() { +fn finds_first_executable_bwrap_in_joined_search_path() { let temp_dir = tempdir().expect("temp dir"); let cwd = temp_dir.path().join("cwd"); let first_dir = temp_dir.path().join("first"); @@ -43,15 +43,16 @@ fn finds_first_executable_bwrap_in_search_paths() { std::fs::create_dir_all(&second_dir).expect("create second dir"); std::fs::write(first_dir.join("bwrap"), "not executable").expect("write non-executable bwrap"); let expected_bwrap = write_named_fake_bwrap_in(&second_dir); + let search_path = std::env::join_paths([first_dir, second_dir]).expect("join search path"); assert_eq!( - find_system_bwrap_in_search_paths(vec![first_dir, second_dir], &cwd), + find_system_bwrap_in_search_paths(std::env::split_paths(&search_path), &cwd), Some(expected_bwrap) ); } #[test] -fn skips_workspace_local_bwrap_in_search_paths() { +fn skips_workspace_local_bwrap_in_joined_search_path() { let temp_dir = tempdir().expect("temp dir"); let cwd = temp_dir.path().join("cwd"); let trusted_dir = temp_dir.path().join("trusted"); @@ -59,9 +60,10 @@ fn skips_workspace_local_bwrap_in_search_paths() { std::fs::create_dir_all(&trusted_dir).expect("create trusted dir"); let _workspace_bwrap = write_named_fake_bwrap_in(&cwd); let expected_bwrap = write_named_fake_bwrap_in(&trusted_dir); + let search_path = std::env::join_paths([cwd.clone(), trusted_dir]).expect("join search path"); assert_eq!( - find_system_bwrap_in_search_paths(vec![cwd.clone(), trusted_dir], &cwd), + find_system_bwrap_in_search_paths(std::env::split_paths(&search_path), &cwd), Some(expected_bwrap) ); }