mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
chore: move bwrap config helpers into dedicated module (#15898)
## Summary - move the bwrap PATH lookup and warning helpers out of config/mod.rs - move the related tests into a dedicated bwrap_tests.rs file ## Validation - git diff --check - skipped heavier local tests per request Follow-up to #15791.
This commit is contained in:
committed by
GitHub
Unverified
parent
609019c6e5
commit
b52abff279
Generated
+1
@@ -2511,6 +2511,7 @@ dependencies = [
|
||||
"tempfile",
|
||||
"tracing",
|
||||
"url",
|
||||
"which 8.0.0",
|
||||
]
|
||||
|
||||
[[package]]
|
||||
|
||||
@@ -5692,118 +5692,6 @@ shell_tool = true
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn system_bwrap_warning_reports_missing_system_bwrap() {
|
||||
let warning =
|
||||
system_bwrap_warning_for_lookup(None).expect("missing system bwrap should emit a warning");
|
||||
|
||||
assert!(warning.contains("could not find system bubblewrap"));
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn system_bwrap_warning_skips_too_old_system_bwrap() {
|
||||
let fake_bwrap = write_fake_bwrap(
|
||||
r#"#!/bin/sh
|
||||
if [ "$1" = "--help" ]; then
|
||||
echo 'usage: bwrap [OPTION...] COMMAND'
|
||||
exit 0
|
||||
fi
|
||||
exit 1
|
||||
"#,
|
||||
);
|
||||
let fake_bwrap_path: &Path = fake_bwrap.as_ref();
|
||||
|
||||
assert_eq!(
|
||||
system_bwrap_warning_for_lookup(Some(fake_bwrap_path.to_path_buf())),
|
||||
None,
|
||||
"Do not warn even if bwrap does not support `--argv0`",
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn finds_first_executable_bwrap_in_search_paths() {
|
||||
let temp_dir = tempdir().expect("temp dir");
|
||||
let cwd = temp_dir.path().join("cwd");
|
||||
let first_dir = temp_dir.path().join("first");
|
||||
let second_dir = temp_dir.path().join("second");
|
||||
std::fs::create_dir_all(&cwd).expect("create cwd");
|
||||
std::fs::create_dir_all(&first_dir).expect("create first dir");
|
||||
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);
|
||||
|
||||
assert_eq!(
|
||||
find_system_bwrap_in_search_paths(vec![first_dir, second_dir], &cwd),
|
||||
Some(expected_bwrap)
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn skips_workspace_local_bwrap_in_search_paths() {
|
||||
let temp_dir = tempdir().expect("temp dir");
|
||||
let cwd = temp_dir.path().join("cwd");
|
||||
let trusted_dir = temp_dir.path().join("trusted");
|
||||
std::fs::create_dir_all(&cwd).expect("create cwd");
|
||||
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);
|
||||
|
||||
assert_eq!(
|
||||
find_system_bwrap_in_search_paths(vec![cwd.clone(), trusted_dir], &cwd),
|
||||
Some(expected_bwrap)
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
#[test]
|
||||
fn system_bwrap_warning_is_disabled_off_linux() {
|
||||
assert!(system_bwrap_warning().is_none());
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn write_fake_bwrap(contents: &str) -> tempfile::TempPath {
|
||||
write_fake_bwrap_in(
|
||||
&std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
contents,
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn write_fake_bwrap_in(dir: &Path, contents: &str) -> tempfile::TempPath {
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
// Bazel can mount the OS temp directory `noexec`, so prefer the current
|
||||
// working directory for fake executables and fall back to the default temp
|
||||
// dir outside that environment.
|
||||
let temp_file = NamedTempFile::new_in(dir)
|
||||
.ok()
|
||||
.unwrap_or_else(|| NamedTempFile::new().expect("temp file"));
|
||||
// Linux rejects exec-ing a file that is still open for writing.
|
||||
let path = temp_file.into_temp_path();
|
||||
fs::write(&path, contents).expect("write fake bwrap");
|
||||
let permissions = fs::Permissions::from_mode(0o755);
|
||||
fs::set_permissions(&path, permissions).expect("chmod fake bwrap");
|
||||
path
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn write_named_fake_bwrap_in(dir: &Path) -> PathBuf {
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let path = dir.join("bwrap");
|
||||
fs::write(&path, "#!/bin/sh\n").expect("write fake bwrap");
|
||||
let permissions = fs::Permissions::from_mode(0o755);
|
||||
fs::set_permissions(&path, permissions).expect("chmod fake bwrap");
|
||||
path
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approvals_reviewer_defaults_to_manual_only_without_guardian_feature() -> std::io::Result<()>
|
||||
{
|
||||
|
||||
@@ -118,7 +118,7 @@ pub use codex_config::Constrained;
|
||||
pub use codex_config::ConstraintError;
|
||||
pub use codex_config::ConstraintResult;
|
||||
pub use codex_network_proxy::NetworkProxyAuditMetadata;
|
||||
|
||||
pub use codex_sandboxing::system_bwrap_warning;
|
||||
pub use managed_features::ManagedFeatures;
|
||||
pub use network_proxy_spec::NetworkProxySpec;
|
||||
pub use network_proxy_spec::StartedNetworkProxy;
|
||||
@@ -144,61 +144,12 @@ pub(crate) const DEFAULT_AGENT_JOB_MAX_RUNTIME_SECONDS: Option<u64> = None;
|
||||
|
||||
pub const CONFIG_TOML_FILE: &str = "config.toml";
|
||||
const OPENAI_BASE_URL_ENV_VAR: &str = "OPENAI_BASE_URL";
|
||||
#[cfg(target_os = "linux")]
|
||||
const SYSTEM_BWRAP_PROGRAM: &str = "bwrap";
|
||||
const RESERVED_MODEL_PROVIDER_IDS: [&str; 3] = [
|
||||
OPENAI_PROVIDER_ID,
|
||||
OLLAMA_OSS_PROVIDER_ID,
|
||||
LMSTUDIO_OSS_PROVIDER_ID,
|
||||
];
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
pub fn system_bwrap_warning() -> Option<String> {
|
||||
system_bwrap_warning_for_lookup(find_system_bwrap_in_path())
|
||||
}
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
pub fn system_bwrap_warning() -> Option<String> {
|
||||
None
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn system_bwrap_warning_for_lookup(system_bwrap_path: Option<PathBuf>) -> Option<String> {
|
||||
match system_bwrap_path {
|
||||
Some(_) => None,
|
||||
None => Some(
|
||||
"Codex could not find system bubblewrap on PATH. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime."
|
||||
.to_string(),
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
pub fn find_system_bwrap_in_path() -> Option<PathBuf> {
|
||||
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)
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
fn find_system_bwrap_in_search_paths(
|
||||
search_paths: impl IntoIterator<Item = PathBuf>,
|
||||
cwd: &Path,
|
||||
) -> Option<PathBuf> {
|
||||
let search_path = std::env::join_paths(search_paths).ok()?;
|
||||
let cwd = std::fs::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf());
|
||||
which::which_in_all(SYSTEM_BWRAP_PROGRAM, Some(search_path), &cwd)
|
||||
.ok()?
|
||||
.find_map(|path| {
|
||||
let path = std::fs::canonicalize(path).ok()?;
|
||||
if path.starts_with(&cwd) {
|
||||
None
|
||||
} else {
|
||||
Some(path)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option<PathBuf> {
|
||||
let raw = std::env::var(codex_state::SQLITE_HOME_ENV).ok()?;
|
||||
let trimmed = raw.trim();
|
||||
@@ -212,6 +163,7 @@ fn resolve_sqlite_home_env(resolved_cwd: &Path) -> Option<PathBuf> {
|
||||
Some(resolved_cwd.join(path))
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
pub(crate) fn test_config() -> Config {
|
||||
let codex_home = tempfile::tempdir().expect("create temp dir");
|
||||
|
||||
@@ -8,6 +8,7 @@ use std::process::Command;
|
||||
use std::sync::OnceLock;
|
||||
|
||||
use crate::vendored_bwrap::exec_vendored_bwrap;
|
||||
use codex_sandboxing::find_system_bwrap_in_path;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -34,7 +35,7 @@ pub(crate) fn exec_bwrap(argv: Vec<String>, preserved_files: Vec<File>) -> ! {
|
||||
fn preferred_bwrap_launcher() -> BubblewrapLauncher {
|
||||
static LAUNCHER: OnceLock<BubblewrapLauncher> = OnceLock::new();
|
||||
LAUNCHER
|
||||
.get_or_init(|| match codex_core::config::find_system_bwrap_in_path() {
|
||||
.get_or_init(|| match find_system_bwrap_in_path() {
|
||||
Some(path) => preferred_bwrap_launcher_for_path(&path),
|
||||
None => BubblewrapLauncher::Vendored,
|
||||
})
|
||||
|
||||
@@ -21,6 +21,7 @@ libc = { workspace = true }
|
||||
serde_json = { workspace = true }
|
||||
tracing = { workspace = true, features = ["log"] }
|
||||
url = { workspace = true }
|
||||
which = { workspace = true }
|
||||
|
||||
[dev-dependencies]
|
||||
pretty_assertions = { workspace = true }
|
||||
|
||||
@@ -0,0 +1,46 @@
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
const SYSTEM_BWRAP_PROGRAM: &str = "bwrap";
|
||||
|
||||
pub fn system_bwrap_warning() -> Option<String> {
|
||||
system_bwrap_warning_for_lookup(find_system_bwrap_in_path())
|
||||
}
|
||||
|
||||
fn system_bwrap_warning_for_lookup(system_bwrap_path: Option<PathBuf>) -> Option<String> {
|
||||
match system_bwrap_path {
|
||||
Some(_) => None,
|
||||
None => Some(
|
||||
"Codex could not find system bubblewrap on PATH. Please install bubblewrap with your package manager. Codex will use the vendored bubblewrap in the meantime."
|
||||
.to_string(),
|
||||
),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn find_system_bwrap_in_path() -> Option<PathBuf> {
|
||||
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)
|
||||
}
|
||||
|
||||
fn find_system_bwrap_in_search_paths(
|
||||
search_paths: impl IntoIterator<Item = PathBuf>,
|
||||
cwd: &Path,
|
||||
) -> Option<PathBuf> {
|
||||
let search_path = std::env::join_paths(search_paths).ok()?;
|
||||
let cwd = std::fs::canonicalize(cwd).unwrap_or_else(|_| cwd.to_path_buf());
|
||||
which::which_in_all(SYSTEM_BWRAP_PROGRAM, Some(search_path), &cwd)
|
||||
.ok()?
|
||||
.find_map(|path| {
|
||||
let path = std::fs::canonicalize(path).ok()?;
|
||||
if path.starts_with(&cwd) {
|
||||
None
|
||||
} else {
|
||||
Some(path)
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "bwrap_tests.rs"]
|
||||
mod tests;
|
||||
@@ -0,0 +1,104 @@
|
||||
use super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::tempdir;
|
||||
|
||||
#[test]
|
||||
fn system_bwrap_warning_reports_missing_system_bwrap() {
|
||||
let warning =
|
||||
system_bwrap_warning_for_lookup(None).expect("missing system bwrap should emit a warning");
|
||||
|
||||
assert!(warning.contains("could not find system bubblewrap"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn system_bwrap_warning_skips_too_old_system_bwrap() {
|
||||
let fake_bwrap = write_fake_bwrap(
|
||||
r#"#!/bin/sh
|
||||
if [ "$1" = "--help" ]; then
|
||||
echo 'usage: bwrap [OPTION...] COMMAND'
|
||||
exit 0
|
||||
fi
|
||||
exit 1
|
||||
"#,
|
||||
);
|
||||
let fake_bwrap_path: &Path = fake_bwrap.as_ref();
|
||||
|
||||
assert_eq!(
|
||||
system_bwrap_warning_for_lookup(Some(fake_bwrap_path.to_path_buf())),
|
||||
None,
|
||||
"Do not warn even if bwrap does not support `--argv0`",
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn finds_first_executable_bwrap_in_search_paths() {
|
||||
let temp_dir = tempdir().expect("temp dir");
|
||||
let cwd = temp_dir.path().join("cwd");
|
||||
let first_dir = temp_dir.path().join("first");
|
||||
let second_dir = temp_dir.path().join("second");
|
||||
std::fs::create_dir_all(&cwd).expect("create cwd");
|
||||
std::fs::create_dir_all(&first_dir).expect("create first dir");
|
||||
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);
|
||||
|
||||
assert_eq!(
|
||||
find_system_bwrap_in_search_paths(vec![first_dir, second_dir], &cwd),
|
||||
Some(expected_bwrap)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn skips_workspace_local_bwrap_in_search_paths() {
|
||||
let temp_dir = tempdir().expect("temp dir");
|
||||
let cwd = temp_dir.path().join("cwd");
|
||||
let trusted_dir = temp_dir.path().join("trusted");
|
||||
std::fs::create_dir_all(&cwd).expect("create cwd");
|
||||
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);
|
||||
|
||||
assert_eq!(
|
||||
find_system_bwrap_in_search_paths(vec![cwd.clone(), trusted_dir], &cwd),
|
||||
Some(expected_bwrap)
|
||||
);
|
||||
}
|
||||
|
||||
fn write_fake_bwrap(contents: &str) -> tempfile::TempPath {
|
||||
write_fake_bwrap_in(
|
||||
&std::env::current_dir().unwrap_or_else(|_| PathBuf::from(".")),
|
||||
contents,
|
||||
)
|
||||
}
|
||||
|
||||
fn write_fake_bwrap_in(dir: &Path, contents: &str) -> tempfile::TempPath {
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
use tempfile::NamedTempFile;
|
||||
|
||||
// Bazel can mount the OS temp directory `noexec`, so prefer the current
|
||||
// working directory for fake executables and fall back to the default temp
|
||||
// dir outside that environment.
|
||||
let temp_file = NamedTempFile::new_in(dir)
|
||||
.ok()
|
||||
.unwrap_or_else(|| NamedTempFile::new().expect("temp file"));
|
||||
// Linux rejects exec-ing a file that is still open for writing.
|
||||
let path = temp_file.into_temp_path();
|
||||
fs::write(&path, contents).expect("write fake bwrap");
|
||||
let permissions = fs::Permissions::from_mode(0o755);
|
||||
fs::set_permissions(&path, permissions).expect("chmod fake bwrap");
|
||||
path
|
||||
}
|
||||
|
||||
fn write_named_fake_bwrap_in(dir: &Path) -> PathBuf {
|
||||
use std::fs;
|
||||
use std::os::unix::fs::PermissionsExt;
|
||||
|
||||
let path = dir.join("bwrap");
|
||||
fs::write(&path, "#!/bin/sh\n").expect("write fake bwrap");
|
||||
let permissions = fs::Permissions::from_mode(0o755);
|
||||
fs::set_permissions(&path, permissions).expect("chmod fake bwrap");
|
||||
path
|
||||
}
|
||||
@@ -1,3 +1,5 @@
|
||||
#[cfg(target_os = "linux")]
|
||||
mod bwrap;
|
||||
pub mod landlock;
|
||||
pub mod macos_permissions;
|
||||
mod manager;
|
||||
@@ -7,6 +9,10 @@ pub mod seatbelt;
|
||||
#[cfg(target_os = "macos")]
|
||||
mod seatbelt_permissions;
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
pub use bwrap::find_system_bwrap_in_path;
|
||||
#[cfg(target_os = "linux")]
|
||||
pub use bwrap::system_bwrap_warning;
|
||||
pub use manager::SandboxCommand;
|
||||
pub use manager::SandboxExecRequest;
|
||||
pub use manager::SandboxManager;
|
||||
@@ -15,3 +21,8 @@ pub use manager::SandboxTransformRequest;
|
||||
pub use manager::SandboxType;
|
||||
pub use manager::SandboxablePreference;
|
||||
pub use manager::get_platform_sandbox;
|
||||
|
||||
#[cfg(not(target_os = "linux"))]
|
||||
pub fn system_bwrap_warning() -> Option<String> {
|
||||
None
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user