From d3812ddbb3b62f46fd9b70fdb96af04d45556e78 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Mon, 14 Sep 2026 18:43:21 +0000 Subject: [PATCH] Make the Guardian deadline cancellation helper crate-private (#45493) ## What changed Restrict `run_before_review_deadline_with_cancel` and its re-export to `codex-guardian-reviewer`. Move its timeout, abort, and successful-completion tests from core into the reviewer's deadline module, and remove the standalone `run_before_review_deadline` tests from core. GitOrigin-RevId: dd9f1ed571a40a4bd66b08c88f3ee2be071f4870 --- codex-rs/core/src/guardian/review_session.rs | 2 - .../core/src/guardian/review_session_tests.rs | 101 ------------------ .../ext/guardian-reviewer/src/deadline.rs | 6 +- .../guardian-reviewer/src/deadline_tests.rs | 69 ++++++++++++ codex-rs/ext/guardian-reviewer/src/lib.rs | 2 +- 5 files changed, 75 insertions(+), 105 deletions(-) create mode 100644 codex-rs/ext/guardian-reviewer/src/deadline_tests.rs diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 9f49f22002..5ba2bd403d 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -100,8 +100,6 @@ use super::prompt::build_guardian_prompt_items_with_parent_turn; use super::review::guardian_review_session_config; pub(crate) use super::reviewer_config::build_guardian_review_session_config; use codex_guardian_reviewer::run_before_review_deadline; -#[cfg(test)] -use codex_guardian_reviewer::run_before_review_deadline_with_cancel; use codex_guardian_reviewer::wait_for_guardian_review; const GUARDIAN_MAX_IMAGE_ITEM_TOKENS: i64 = 10_000; diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index b76805bc52..cda680ac5e 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -775,107 +775,6 @@ async fn guardian_review_session_config_preserves_explicit_empty_catalog_templat ); } -#[tokio::test(flavor = "current_thread")] -async fn run_before_review_deadline_times_out_before_future_completes() { - let outcome = run_before_review_deadline( - tokio::time::Instant::now() + Duration::from_millis(10), - /*external_cancel*/ None, - async { - tokio::time::sleep(Duration::from_millis(50)).await; - }, - ) - .await; - - assert!(matches!( - outcome, - Err(GuardianReviewSessionOutcome::TimedOut) - )); -} - -#[tokio::test(flavor = "current_thread")] -async fn run_before_review_deadline_aborts_when_cancelled() { - let cancel_token = CancellationToken::new(); - let canceller = cancel_token.clone(); - drop(tokio::spawn(async move { - tokio::time::sleep(Duration::from_millis(10)).await; - canceller.cancel(); - })); - - let outcome = run_before_review_deadline( - tokio::time::Instant::now() + Duration::from_secs(1), - Some(&cancel_token), - std::future::pending::<()>(), - ) - .await; - - assert!(matches!( - outcome, - Err(GuardianReviewSessionOutcome::Aborted) - )); -} - -#[tokio::test(flavor = "current_thread")] -async fn run_before_review_deadline_with_cancel_cancels_token_on_timeout() { - let cancel_token = CancellationToken::new(); - - let outcome = run_before_review_deadline_with_cancel( - tokio::time::Instant::now() + Duration::from_millis(10), - /*external_cancel*/ None, - &cancel_token, - async { - tokio::time::sleep(Duration::from_millis(50)).await; - }, - ) - .await; - - assert!(matches!( - outcome, - Err(GuardianReviewSessionOutcome::TimedOut) - )); - assert!(cancel_token.is_cancelled()); -} - -#[tokio::test(flavor = "current_thread")] -async fn run_before_review_deadline_with_cancel_cancels_token_on_abort() { - let external_cancel = CancellationToken::new(); - let external_canceller = external_cancel.clone(); - let cancel_token = CancellationToken::new(); - drop(tokio::spawn(async move { - tokio::time::sleep(Duration::from_millis(10)).await; - external_canceller.cancel(); - })); - - let outcome = run_before_review_deadline_with_cancel( - tokio::time::Instant::now() + Duration::from_secs(1), - Some(&external_cancel), - &cancel_token, - std::future::pending::<()>(), - ) - .await; - - assert!(matches!( - outcome, - Err(GuardianReviewSessionOutcome::Aborted) - )); - assert!(cancel_token.is_cancelled()); -} - -#[tokio::test(flavor = "current_thread")] -async fn run_before_review_deadline_with_cancel_preserves_token_on_success() { - let cancel_token = CancellationToken::new(); - - let outcome = run_before_review_deadline_with_cancel( - tokio::time::Instant::now() + Duration::from_secs(1), - /*external_cancel*/ None, - &cancel_token, - async { 42usize }, - ) - .await; - - assert_eq!(outcome.unwrap(), 42); - assert!(!cancel_token.is_cancelled()); -} - #[test] fn had_prior_review_context_tracks_prompt_mode() { assert!(!had_prior_review_context(&GuardianPromptMode::Full)); diff --git a/codex-rs/ext/guardian-reviewer/src/deadline.rs b/codex-rs/ext/guardian-reviewer/src/deadline.rs index 6a91bfac5d..b7b63b8012 100644 --- a/codex-rs/ext/guardian-reviewer/src/deadline.rs +++ b/codex-rs/ext/guardian-reviewer/src/deadline.rs @@ -22,7 +22,7 @@ pub async fn run_before_review_deadline( } } -pub async fn run_before_review_deadline_with_cancel( +pub(crate) async fn run_before_review_deadline_with_cancel( deadline: tokio::time::Instant, external_cancel: Option<&CancellationToken>, cancel_token: &CancellationToken, @@ -34,3 +34,7 @@ pub async fn run_before_review_deadline_with_cancel( } result } + +#[cfg(test)] +#[path = "deadline_tests.rs"] +mod tests; diff --git a/codex-rs/ext/guardian-reviewer/src/deadline_tests.rs b/codex-rs/ext/guardian-reviewer/src/deadline_tests.rs new file mode 100644 index 0000000000..df3a5d1b22 --- /dev/null +++ b/codex-rs/ext/guardian-reviewer/src/deadline_tests.rs @@ -0,0 +1,69 @@ +use std::time::Duration; + +use pretty_assertions::assert_eq; +use tokio_util::sync::CancellationToken; + +use super::run_before_review_deadline_with_cancel; +use crate::GuardianReviewSessionOutcome; + +#[tokio::test(flavor = "current_thread")] +async fn run_before_review_deadline_with_cancel_cancels_token_on_timeout() { + let cancel_token = CancellationToken::new(); + + let outcome = run_before_review_deadline_with_cancel( + tokio::time::Instant::now() + Duration::from_millis(10), + /*external_cancel*/ None, + &cancel_token, + async { + tokio::time::sleep(Duration::from_millis(50)).await; + }, + ) + .await; + + assert!(matches!( + outcome, + Err(GuardianReviewSessionOutcome::TimedOut) + )); + assert!(cancel_token.is_cancelled()); +} + +#[tokio::test(flavor = "current_thread")] +async fn run_before_review_deadline_with_cancel_cancels_token_on_abort() { + let external_cancel = CancellationToken::new(); + let external_canceller = external_cancel.clone(); + let cancel_token = CancellationToken::new(); + drop(tokio::spawn(async move { + tokio::time::sleep(Duration::from_millis(10)).await; + external_canceller.cancel(); + })); + + let outcome = run_before_review_deadline_with_cancel( + tokio::time::Instant::now() + Duration::from_secs(1), + Some(&external_cancel), + &cancel_token, + std::future::pending::<()>(), + ) + .await; + + assert!(matches!( + outcome, + Err(GuardianReviewSessionOutcome::Aborted) + )); + assert!(cancel_token.is_cancelled()); +} + +#[tokio::test(flavor = "current_thread")] +async fn run_before_review_deadline_with_cancel_preserves_token_on_success() { + let cancel_token = CancellationToken::new(); + + let outcome = run_before_review_deadline_with_cancel( + tokio::time::Instant::now() + Duration::from_secs(1), + /*external_cancel*/ None, + &cancel_token, + async { 42usize }, + ) + .await; + + assert_eq!(outcome.unwrap(), 42); + assert!(!cancel_token.is_cancelled()); +} diff --git a/codex-rs/ext/guardian-reviewer/src/lib.rs b/codex-rs/ext/guardian-reviewer/src/lib.rs index c41cb64e10..61748f92e8 100644 --- a/codex-rs/ext/guardian-reviewer/src/lib.rs +++ b/codex-rs/ext/guardian-reviewer/src/lib.rs @@ -40,7 +40,7 @@ pub const MAX_REVIEW_ATTEMPTS: i64 = 3; pub const REVIEW_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(90); pub use deadline::run_before_review_deadline; -pub use deadline::run_before_review_deadline_with_cancel; +pub(crate) use deadline::run_before_review_deadline_with_cancel; pub use pool::ReviewerPool; pub use pool::ReviewerRequest; pub use pool::ReviewerSession;