From 76c3ea52f94ed4842948fd0fe6a7a7421f2c3a7a Mon Sep 17 00:00:00 2001 From: Chris Bookholt Date: Thu, 2 Jul 2026 11:30:33 -0700 Subject: [PATCH] test(git-utils): decouple policy checks from status SLA --- codex-rs/git-utils/Cargo.toml | 1 + codex-rs/git-utils/src/info.rs | 131 ++++++++++++++++++++++- codex-rs/git-utils/src/safe_git_tests.rs | 38 ++++--- 3 files changed, 155 insertions(+), 15 deletions(-) diff --git a/codex-rs/git-utils/Cargo.toml b/codex-rs/git-utils/Cargo.toml index 419eb7d1b8..7d137de8f6 100644 --- a/codex-rs/git-utils/Cargo.toml +++ b/codex-rs/git-utils/Cargo.toml @@ -35,6 +35,7 @@ walkdir = { workspace = true } [dev-dependencies] pretty_assertions = { workspace = true } +tokio = { workspace = true, features = ["test-util"] } [lib] doctest = false diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index dd8328fc54..265b691316 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -1,10 +1,12 @@ use std::collections::BTreeMap; use std::collections::HashSet; use std::ffi::OsStr; +use std::future::Future; use std::path::Path; use std::path::PathBuf; use std::sync::atomic::AtomicU8; use std::sync::atomic::Ordering; +use std::time::Duration; use codex_file_system::ExecutorFileSystem; use codex_file_system::FindUpErrorPolicy; @@ -301,12 +303,23 @@ pub async fn try_get_has_changes(cwd: &Path) -> Result { // It cancels active Git children through the runner's kill-on-drop handle; // synchronous filesystem authority checks are not preemptible mid-call. let phase = AtomicU8::new(StatusReadPhase::Preparation as u8); - match timeout( + run_status_read_with_timeout( GIT_COMMAND_TIMEOUT, - Box::pin(try_get_has_changes_within_deadline(cwd, &phase)), + &phase, + try_get_has_changes_within_deadline(cwd, &phase), ) .await - { +} + +async fn run_status_read_with_timeout( + duration: Duration, + phase: &AtomicU8, + status_read: F, +) -> Result +where + F: Future>, +{ + match timeout(duration, status_read).await { Ok(result) => result, Err(_) => Err(GitReadError::CommandTimedOut { operation: StatusReadPhase::from_u8(phase.load(Ordering::Relaxed)) @@ -316,6 +329,24 @@ pub async fn try_get_has_changes(cwd: &Path) -> Result { } } +#[cfg(test)] +const TEST_STATUS_READ_TIMEOUT: Duration = Duration::from_secs(60); + +/// Run the production status pipeline with a generous safety deadline for +/// semantic tests whose assertions should not depend on the product SLA. +#[cfg(test)] +pub(crate) async fn try_get_has_changes_with_test_timeout( + cwd: &Path, +) -> Result { + let phase = AtomicU8::new(StatusReadPhase::Preparation as u8); + run_status_read_with_timeout( + TEST_STATUS_READ_TIMEOUT, + &phase, + try_get_has_changes_within_deadline(cwd, &phase), + ) + .await +} + #[derive(Clone, Copy)] #[repr(u8)] enum StatusReadPhase { @@ -971,6 +1002,100 @@ mod tests { use pretty_assertions::assert_eq; #[cfg(unix)] use std::os::unix::fs::PermissionsExt; + use std::sync::Arc; + use std::sync::atomic::AtomicBool; + + #[tokio::test(start_paused = true)] + async fn status_read_timeout_passes_through_ready_ok() { + let phase = AtomicU8::new(StatusReadPhase::Preparation as u8); + + assert_eq!( + run_status_read_with_timeout(Duration::from_secs(1), &phase, async { Ok(false) }).await, + Ok(false) + ); + } + + #[tokio::test(start_paused = true)] + async fn status_read_timeout_passes_through_ready_error() { + let phase = AtomicU8::new(StatusReadPhase::Preparation as u8); + let error = GitReadError::InvalidOutput { + operation: "testOperation".to_string(), + }; + + assert_eq!( + run_status_read_with_timeout(Duration::from_secs(1), &phase, async { + Err::(error.clone()) + }) + .await, + Err(error) + ); + } + + #[tokio::test(start_paused = true)] + async fn status_read_timeout_maps_every_pending_phase() { + for (phase_value, operation) in [ + (StatusReadPhase::Preparation, "statusFilterPreparation"), + (StatusReadPhase::Fsmonitor, "statusFsmonitor"), + (StatusReadPhase::Status, "status"), + ] { + let phase = AtomicU8::new(phase_value as u8); + + assert_eq!( + run_status_read_with_timeout( + Duration::from_secs(1), + &phase, + std::future::pending::>(), + ) + .await, + Err(GitReadError::CommandTimedOut { + operation: operation.to_string(), + }) + ); + } + } + + #[tokio::test(start_paused = true)] + async fn status_read_timeout_uses_live_phase_transition() { + let phase = AtomicU8::new(StatusReadPhase::Preparation as u8); + + assert_eq!( + run_status_read_with_timeout(Duration::from_secs(1), &phase, async { + phase.store(StatusReadPhase::Fsmonitor as u8, Ordering::Relaxed); + std::future::pending::>().await + }) + .await, + Err(GitReadError::CommandTimedOut { + operation: "statusFsmonitor".to_string(), + }) + ); + } + + #[tokio::test(start_paused = true)] + async fn status_read_timeout_drops_pending_future() { + struct DropSentinel(Arc); + + impl Drop for DropSentinel { + fn drop(&mut self) { + self.0.store(true, Ordering::Relaxed); + } + } + + let phase = AtomicU8::new(StatusReadPhase::Preparation as u8); + let dropped = Arc::new(AtomicBool::new(false)); + let sentinel = DropSentinel(Arc::clone(&dropped)); + + assert_eq!( + run_status_read_with_timeout(Duration::from_secs(1), &phase, async move { + let _sentinel = sentinel; + std::future::pending::>().await + }) + .await, + Err(GitReadError::CommandTimedOut { + operation: "statusFilterPreparation".to_string(), + }) + ); + assert!(dropped.load(Ordering::Relaxed)); + } #[cfg(unix)] fn run_git(cwd: &Path, args: &[&str]) -> std::process::Output { diff --git a/codex-rs/git-utils/src/safe_git_tests.rs b/codex-rs/git-utils/src/safe_git_tests.rs index b503c3c07e..66639bdc2d 100644 --- a/codex-rs/git-utils/src/safe_git_tests.rs +++ b/codex-rs/git-utils/src/safe_git_tests.rs @@ -2,10 +2,10 @@ use super::*; use crate::GitReadError; use crate::apply::ApplyGitRequest; use crate::apply::apply_git_patch; -use crate::get_has_changes; use crate::git_command::GitRunner; use crate::git_config::GitConfigScope; use crate::guarded_config::GuardedGitConfig; +use crate::info::try_get_has_changes_with_test_timeout; #[cfg(unix)] use crate::patch_paths::stage_paths; #[cfg(unix)] @@ -660,9 +660,8 @@ async fn get_has_changes_rejects_configured_clean_filter_without_running_it() { let repo_path = create_test_git_repo(&temp_dir).await; configure_clean_filter(&repo_path, "test.txt").await; - assert_eq!(get_has_changes(&repo_path).await, None); assert_eq!( - try_get_has_changes(&repo_path).await, + try_get_has_changes_with_test_timeout(&repo_path).await, Err(GitReadError::SelectedExecutableFilter { driver: "x=y".to_string(), path: "test.txt".to_string(), @@ -699,7 +698,11 @@ async fn get_has_changes_distinguishes_filter_sentinels_from_literal_driver_name run_git_async(&repo_path, &["add", ".gitattributes"]).await; run_git_async(&repo_path, &["commit", "-m", "sentinel attribute"]).await; - assert_eq!(try_get_has_changes(&repo_path).await, Ok(false), "{driver}"); + assert_eq!( + try_get_has_changes_with_test_timeout(&repo_path).await, + Ok(false), + "{driver}" + ); assert!(!configured_filter_ran(&repo_path).await, "{driver}"); std::fs::write( @@ -708,7 +711,7 @@ async fn get_has_changes_distinguishes_filter_sentinels_from_literal_driver_name ) .expect("write literal sentinel-named driver"); assert_eq!( - try_get_has_changes(&repo_path).await, + try_get_has_changes_with_test_timeout(&repo_path).await, Err(GitReadError::SelectedExecutableFilter { driver: driver.to_string(), path: "test.txt".to_string(), @@ -743,7 +746,10 @@ async fn checked_has_changes_accepts_non_utf8_repository_root() { std::fs::create_dir(&repo_path).expect("create non-UTF-8 repository directory"); run_git_async(&repo_path, &["init"]).await; - assert_eq!(try_get_has_changes(&repo_path).await, Ok(false)); + assert_eq!( + try_get_has_changes_with_test_timeout(&repo_path).await, + Ok(false) + ); } #[cfg(unix)] @@ -792,7 +798,7 @@ async fn checked_has_changes_preserves_lexical_repository_ancestry_for_git_selec std::env::var_os("CODEX_GIT_UTILS_PHYSICAL_REPO").expect("physical repository"), ); assert_eq!( - try_get_has_changes(&physical_nested).await, + try_get_has_changes_with_test_timeout(&physical_nested).await, Err(GitReadError::CommandFailed { operation: "statusFilterPreparation".to_string(), exit_code: None, @@ -802,7 +808,7 @@ async fn checked_has_changes_preserves_lexical_repository_ancestry_for_git_selec let lexical_nested = PathBuf::from(std::env::var_os("CODEX_GIT_UTILS_TARGET_REPO").expect("target repository")); assert_eq!( - try_get_has_changes(&lexical_nested).await, + try_get_has_changes_with_test_timeout(&lexical_nested).await, Err(GitReadError::NoTrustedGit), "the lexical spelling must exclude the enclosing repository's native Git decoy" ); @@ -931,9 +937,8 @@ async fn get_has_changes_rejects_core_worktree_redirection_before_running_filter ) .await; - assert_eq!(get_has_changes(&repo_path).await, None); assert_eq!( - try_get_has_changes(&repo_path).await, + try_get_has_changes_with_test_timeout(&repo_path).await, Err(GitReadError::RepositoryRootMismatch { expected_root: std::fs::canonicalize(&repo_path).expect("canonical repository"), reported_root: std::fs::canonicalize(&redirected).expect("canonical redirection"), @@ -994,7 +999,13 @@ async fn legacy_git_config_cannot_hide_a_selected_local_filter_from_probe() { let repo_path = PathBuf::from(std::env::var_os("CODEX_GIT_UTILS_TARGET_REPO").expect("target repository")); - assert_eq!(get_has_changes(&repo_path).await, None); + assert_eq!( + try_get_has_changes_with_test_timeout(&repo_path).await, + Err(GitReadError::SelectedExecutableFilter { + driver: "evil".to_string(), + path: "test.txt".to_string(), + }) + ); } #[tokio::test] @@ -1003,7 +1014,10 @@ async fn get_has_changes_does_not_enter_dirty_submodules() { let repo_path = create_test_git_repo(&temp_dir).await; add_submodule_with_clean_filter(&repo_path).await; - assert_eq!(get_has_changes(&repo_path).await, Some(false)); + assert_eq!( + try_get_has_changes_with_test_timeout(&repo_path).await, + Ok(false) + ); assert!(!configured_filter_ran(&repo_path.join("nested")).await); }