From 16845bb8e85d781f6e745bdec011384450929ff9 Mon Sep 17 00:00:00 2001 From: jif Date: Mon, 12 Oct 2026 15:33:26 +0000 Subject: [PATCH] Isolate arg0 helper directories by operating system (#53412) ## Why Windows and WSL can share `CODEX_HOME` without sharing a lock namespace, allowing cleanup to remove another platform's live helpers. Older clients can also clean helpers stored under the shared `tmp/arg0` root. ## What changed Create helpers under `CODEX_HOME/tmp/arg0-` and limit stale-directory cleanup to that operating system's root. Keep permission discovery compatible with both the current platform's root and legacy `tmp/arg0` directories, while ignoring other platforms' aliases. ## Testing Add regression coverage for cleanup isolation, live helper preservation during legacy cleanup, and selection of the first session alias directory from either supported root. GitOrigin-RevId: f34292748b5b1d3c25629604d849db4b133dfa3a --- codex-rs/arg0/src/lib.rs | 61 ++++++++++++++++++- codex-rs/core/src/config/permissions.rs | 8 ++- codex-rs/core/src/config/permissions_tests.rs | 39 ++++++++---- 3 files changed, 90 insertions(+), 18 deletions(-) diff --git a/codex-rs/arg0/src/lib.rs b/codex-rs/arg0/src/lib.rs index 33d2e65989..31ed3723c2 100644 --- a/codex-rs/arg0/src/lib.rs +++ b/codex-rs/arg0/src/lib.rs @@ -324,9 +324,21 @@ fn prepare_path_entry_for_codex_aliases( } } - std::fs::create_dir_all(&codex_home)?; - // Use a CODEX_HOME-scoped temp root to avoid cluttering the top-level directory. - let temp_root = codex_home.join("tmp").join("arg0"); + prepare_aliases_in_home(&codex_home, existing_path) +} + +// The production entry point validates the home before reaching this filesystem seam. +fn prepare_aliases_in_home( + codex_home: &Path, + existing_path: Option, +) -> std::io::Result<(Arg0PathEntryGuard, OsString)> { + std::fs::create_dir_all(codex_home)?; + // Windows and WSL can share CODEX_HOME without sharing a lock namespace. + // Keep each OS outside the legacy `tmp/arg0` root so older clients cannot + // delete these helpers while their owning process is still running. + let temp_root = codex_home + .join("tmp") + .join(format!("arg0-{}", std::env::consts::OS)); std::fs::create_dir_all(&temp_root)?; #[cfg(unix)] { @@ -725,6 +737,49 @@ mod tests { )) } + #[test] + fn helper_cleanup_is_isolated_from_other_platforms_and_legacy_clients() -> std::io::Result<()> { + let home = tempfile::tempdir()?; + let temporary = home.path().join("tmp"); + let own_root = temporary.join(format!("arg0-{}", std::env::consts::OS)); + let foreign_os = if cfg!(windows) { "linux" } else { "windows" }; + let foreign_root = temporary.join(format!("arg0-{foreign_os}")); + let legacy_root = temporary.join("arg0"); + // Unlocked foreign directories model a live peer whose lock is invisible + // to this kernel. Only stale directories in our own namespace may go. + for root in [&own_root, &foreign_root, &legacy_root] { + let stale = root.join("codex-arg0-existing"); + fs::create_dir_all(&stale)?; + create_lock(&stale)?; + fs::write(stale.join("helper"), b"keep foreign helpers")?; + } + + let (guard, _) = super::prepare_aliases_in_home(home.path(), /*existing_path*/ None)?; + assert!(!own_root.join("codex-arg0-existing").exists()); + assert_eq!( + [&foreign_root, &legacy_root] + .map(|root| fs::read(root.join("codex-arg0-existing/helper"))) + .into_iter() + .collect::>>()?, + vec![b"keep foreign helpers".to_vec(); 2], + ); + + // Older versions clean only this root. Run that cleanup while a new + // helper is live, then confirm the advertised helper remains usable. + janitor_cleanup(&legacy_root)?; + let helper = guard._temp_dir.path().join(if cfg!(windows) { + "apply_patch.bat" + } else { + "apply_patch" + }); + assert!(helper.is_file()); + assert_eq!( + fs::read(foreign_root.join("codex-arg0-existing/helper"))?, + b"keep foreign helpers", + ); + Ok(()) + } + #[test] fn janitor_skips_dirs_without_lock_file() -> std::io::Result<()> { let root = tempfile::tempdir()?; diff --git a/codex-rs/core/src/config/permissions.rs b/codex-rs/core/src/config/permissions.rs index 78eaf3d217..bd6ddb1f09 100644 --- a/codex-rs/core/src/config/permissions.rs +++ b/codex-rs/core/src/config/permissions.rs @@ -54,10 +54,14 @@ pub(super) fn active_arg0_helper_dir( codex_home: &Path, path_env: &OsStr, ) -> Option { - let arg0_root = codex_home.join("tmp").join("arg0"); + let arg0_root = codex_home + .join("tmp") + .join(format!("arg0-{}", std::env::consts::OS)); + let legacy_root = codex_home.join("tmp").join("arg0"); std::env::split_paths(path_env) .find(|path| { - path.parent() == Some(arg0_root.as_path()) + (path.parent() == Some(arg0_root.as_path()) + || path.parent() == Some(legacy_root.as_path())) && path .file_name() .is_some_and(|name| name.to_string_lossy().starts_with("codex-arg0")) diff --git a/codex-rs/core/src/config/permissions_tests.rs b/codex-rs/core/src/config/permissions_tests.rs index 0e2468b8b1..b3c5d9bcb7 100644 --- a/codex-rs/core/src/config/permissions_tests.rs +++ b/codex-rs/core/src/config/permissions_tests.rs @@ -22,19 +22,32 @@ use tempfile::TempDir; #[test] fn active_arg0_helper_dir_selects_only_the_first_session_alias_directory() -> anyhow::Result<()> { let home = TempDir::new()?; - let arg0_root = home.path().join("tmp/arg0"); - let active = arg0_root.join("codex-arg0-active"); - let paths = std::env::join_paths([ - home.path().join("bin"), - arg0_root.clone(), - arg0_root.join("unrelated"), - active.clone(), - arg0_root.join("codex-arg0-parent"), - ])?; - assert_eq!( - active_arg0_helper_dir(home.path(), &paths), - Some(AbsolutePathBuf::from_absolute_path(active)?) - ); + let platform_root = home + .path() + .join("tmp") + .join(format!("arg0-{}", std::env::consts::OS)); + let legacy_root = home.path().join("tmp/arg0"); + let foreign_os = if cfg!(windows) { "linux" } else { "windows" }; + let foreign_alias = home + .path() + .join("tmp") + .join(format!("arg0-{foreign_os}")) + .join("codex-arg0-foreign"); + for arg0_root in [&platform_root, &legacy_root] { + let active = arg0_root.join("codex-arg0-active"); + let paths = std::env::join_paths([ + home.path().join("bin"), + foreign_alias.clone(), + arg0_root.clone(), + arg0_root.join("unrelated"), + active.clone(), + arg0_root.join("codex-arg0-parent"), + ])?; + assert_eq!( + active_arg0_helper_dir(home.path(), &paths), + Some(AbsolutePathBuf::from_absolute_path(active)?) + ); + } Ok(()) }