openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-05 20:10:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

test(git-utils): decouple policy checks from status SLA

Browse Source
This commit is contained in:
Chris Bookholt
2026-07-02 11:30:33 -07:00
parent abc39f017c
commit 76c3ea52f9
3 changed files with 155 additions and 15 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/git-utils/Cargo.toml
+1
View File
@@ -35,6 +35,7 @@ walkdir = { workspace = true }
[dev-dependencies]
pretty_assertions = { workspace = true }
tokio = { workspace = true, features = ["test-util"] }
[lib]
doctest = false
codex-rs/git-utils/src/info.rs
+128 -3
View File
@@ -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<bool, GitReadError> {
// 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<F, T>(
duration: Duration,
phase: &AtomicU8,
status_read: F,
) -> Result<T, GitReadError>
where
F: Future<Output = Result<T, GitReadError>>,
{
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<bool, GitReadError> {
}
}
#[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<bool, GitReadError> {
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::<bool, _>(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::<Result<bool, GitReadError>>(),
)
.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::<Result<bool, GitReadError>>().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<AtomicBool>);
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::<Result<bool, GitReadError>>().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 {
codex-rs/git-utils/src/safe_git_tests.rs
+26 -12
View File
@@ -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);
}