From 4701aa4b4239c70063ab6f2fcb835324f9c109f4 Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 16 Sep 2026 12:01:07 +0000 Subject: [PATCH] Avoid redundant model catalog lookups in reused Guardian reviewers (#45933) ## Why Reusing a Guardian reviewer could trigger a model catalog refresh during the previous-model compaction check, even when the model was unchanged and no compaction compatibility change required compaction. ## What changed Return early from the previous-model compaction check for Guardian sessions when the model slug is unchanged and the compaction compatibility hashes do not require compaction. ## Testing Add a regression test with an expired model catalog cache that verifies two approvals reuse the same Guardian reviewer, both reviews and the parent turn complete, and no additional model catalog request occurs. GitOrigin-RevId: cc1cdc5f89c6f9ec03f1bedfb78cad9780825733 --- codex-rs/core/src/session/turn.rs | 6 + codex-rs/core/tests/suite/models_cache_ttl.rs | 106 ++++++++++++++++++ 2 files changed, 112 insertions(+) diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 1f80548e4e..ba9a4f02f1 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -1321,6 +1321,12 @@ async fn maybe_run_previous_model_inline_compact( turn_context.model_info().comp_hash.as_deref(), ); let previous_model = previous_turn_settings.model; + if crate::guardian::is_basic_session_source(&turn_context.session_source) + && !should_compact_for_comp_hash_change + && previous_model == turn_context.model_info().slug + { + return Ok(()); + } let previous_model_turn_context = Arc::new( turn_context .with_model(previous_model.clone(), &sess.services.models_manager) diff --git a/codex-rs/core/tests/suite/models_cache_ttl.rs b/codex-rs/core/tests/suite/models_cache_ttl.rs index a50a509766..3cba8b6eff 100644 --- a/codex-rs/core/tests/suite/models_cache_ttl.rs +++ b/codex-rs/core/tests/suite/models_cache_ttl.rs @@ -7,9 +7,11 @@ use anyhow::Result; use chrono::DateTime; use chrono::TimeZone; use chrono::Utc; +use codex_core::config::Constrained; use codex_login::CodexAuth; use codex_models_manager::client_version_to_whole; use codex_models_manager::manager::RefreshStrategy; +use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::ReasoningSummary; @@ -24,15 +26,19 @@ use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::openai_models::TruncationPolicyConfig; use codex_protocol::openai_models::default_input_modalities; +use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::user_input::UserInput; use core_test_support::responses; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_response_created; +use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::sse; use core_test_support::responses::sse_response; +use core_test_support::skip_if_no_network; use core_test_support::test_codex::test_codex; use core_test_support::test_codex::turn_permission_fields; use core_test_support::wait_for_event; @@ -49,6 +55,106 @@ const VERSIONED_MODEL: &str = "codex-test-versioned"; const MISSING_VERSION_MODEL: &str = "codex-test-missing-version"; const DIFFERENT_VERSION_MODEL: &str = "codex-test-different-version"; +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn guardian_reused_reviewer_avoids_stale_catalog_lookup() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = MockServer::start().await; + let bundled = codex_models_manager::bundled_models_response()?; + let catalog = ModelsResponse { + models: bundled + .models + .into_iter() + .filter(|model| ["gpt-5.4", "codex-auto-review"].contains(&model.slug.as_str())) + .collect(), + }; + assert_eq!(catalog.models.len(), 2); + let initial_models = + responses::mount_models_once_with_etag(&server, catalog.clone(), ETAG).await; + + let mut builder = test_codex() + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_model("gpt-5.4"); + builder = builder.with_config(|config| { + config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest); + config.approvals_reviewer = ApprovalsReviewer::AutoReview; + config.model_provider.request_max_retries = Some(0); + }); + let test = builder.build_with_auto_env(&server).await?; + test.thread_manager + .get_models_manager() + .list_models( + RefreshStrategy::OnlineIfUncached, + codex_core::test_support::default_http_client_factory(), + ) + .await; + assert_eq!(initial_models.requests().len(), 1); + rewrite_cache_timestamp( + &test.config.codex_home.join(CACHE_FILE), + Utc::now() - chrono::Duration::hours(1), + ) + .await?; + let repeat_models = responses::mount_models_once_with_etag(&server, catalog, ETAG).await; + let deny = r#"{"risk_level":"high","user_authorization":"low","outcome":"deny"}"#; + let mut sequence = Vec::new(); + for index in 1..=2 { + let parent = format!("parent-{index}"); + let guardian = format!("guardian-{index}"); + sequence.push(sse(vec![ + ev_response_created(&parent), + ev_function_call( + &format!("call-{index}"), + "exec_command", + r#"{"cmd":"true","sandbox_permissions":"require_escalated","justification":"Check an action before running it."}"#, + ), + ev_completed(&parent), + ])); + sequence.push(sse(vec![ + ev_response_created(&guardian), + ev_assistant_message(&guardian, deny), + ev_completed(&guardian), + ])); + } + sequence.push(sse(vec![ + ev_response_created("parent-done"), + ev_assistant_message("parent-done", "finished"), + ev_completed("parent-done"), + ])); + let response_mock = mount_sse_sequence(&server, sequence).await; + test.submit_text_turn("Review both shell requests.").await?; + + let requests = response_mock.requests(); + assert_eq!( + requests.len(), + 5, + "both reviews and the parent turn must finish" + ); + let guardian_requests = requests + .iter() + .filter(|request| { + request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian") + }) + .collect::>(); + assert_eq!(guardian_requests.len(), 2); + for request in &guardian_requests { + assert_eq!( + request.body_json()["model"].as_str(), + Some("codex-auto-review") + ); + } + let guardian_thread = guardian_requests[0].body_json()["client_metadata"]["thread_id"] + .as_str() + .expect("Guardian reviewer thread id") + .to_string(); + assert_eq!( + guardian_requests[1].body_json()["client_metadata"]["thread_id"].as_str(), + Some(guardian_thread.as_str()), + "the second approval must reuse the Guardian reviewer" + ); + assert_eq!(repeat_models.requests().len(), 0); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn renews_cache_ttl_on_matching_models_etag() -> Result<()> { let server = MockServer::start().await;