mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Use Guardian model limits for review sessions (#34847)
## Why Parent context-window and auto-compaction overrides may describe a different model than the one selected for Guardian review. ## What changed - Clear `model_context_window` and `model_auto_compact_token_limit` when the effective Guardian model differs from the parent model, allowing the review model's own limits to apply. - Preserve those overrides when Guardian uses the same effective model. ## Testing Add unit coverage for both model-selection paths and verify that a Guardian rollout records the selected review model's context window. GitOrigin-RevId: ebc399f4eba37c7f279fd0d258154c00d45e2d00
This commit is contained in:
@@ -730,13 +730,17 @@ pub(super) async fn guardian_review_session_config(
|
||||
&turn.config.to_models_manager_config(),
|
||||
)
|
||||
.await;
|
||||
let spawn_config = build_guardian_review_session_config(
|
||||
let mut spawn_config = build_guardian_review_session_config(
|
||||
turn.config.as_ref(),
|
||||
live_network_config,
|
||||
guardian_model.as_str(),
|
||||
guardian_reasoning_effort.clone(),
|
||||
guardian_model_info.model_messages.as_ref(),
|
||||
)?;
|
||||
if guardian_model != turn.model_info.slug {
|
||||
spawn_config.model_context_window = None;
|
||||
spawn_config.model_auto_compact_token_limit = None;
|
||||
}
|
||||
Ok(GuardianReviewSessionConfig {
|
||||
spawn_config,
|
||||
model: guardian_model,
|
||||
|
||||
@@ -9,6 +9,7 @@ use crate::guardian::approval_request::guardian_request_target_item_id;
|
||||
use crate::guardian::prompt::BUNDLED_GUARDIAN_POLICY;
|
||||
use crate::guardian::prompt::BUNDLED_GUARDIAN_POLICY_TEMPLATE;
|
||||
use crate::guardian::prompt::guardian_policy_prompt_with_config_and_template;
|
||||
use crate::guardian::review::guardian_review_session_config;
|
||||
use crate::session::session::Session;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::test_support;
|
||||
@@ -2932,6 +2933,69 @@ async fn guardian_review_session_config_preserves_parent_network_proxy() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_session_config_clears_context_overrides_for_distinct_effective_model() {
|
||||
let server = start_mock_server().await;
|
||||
let (session, mut turn) = guardian_test_session_and_turn(&server).await;
|
||||
let mut config = (*turn.config).clone();
|
||||
config.model = Some("codex-auto-review".to_string());
|
||||
config.model_context_window = Some(900_000);
|
||||
config.model_auto_compact_token_limit = Some(600_000);
|
||||
Arc::get_mut(&mut turn)
|
||||
.expect("turn should be unique")
|
||||
.config = Arc::new(config);
|
||||
|
||||
let guardian_config = guardian_review_session_config(session.as_ref(), turn.as_ref())
|
||||
.await
|
||||
.expect("guardian config")
|
||||
.spawn_config;
|
||||
|
||||
assert_eq!(
|
||||
(
|
||||
guardian_config.model_context_window,
|
||||
guardian_config.model_auto_compact_token_limit,
|
||||
),
|
||||
(None, None)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_session_config_preserves_context_overrides_for_same_effective_model() {
|
||||
let server = start_mock_server().await;
|
||||
let (mut session, mut turn) = guardian_test_session_and_turn(&server).await;
|
||||
let parent_model = turn.model_info.clone();
|
||||
let auth_manager = Arc::clone(&session.services.auth_manager);
|
||||
Arc::get_mut(&mut session)
|
||||
.expect("session should be unique")
|
||||
.services
|
||||
.models_manager = Arc::new(StaticModelsManager::new(
|
||||
Some(auth_manager),
|
||||
ModelsResponse {
|
||||
models: vec![parent_model],
|
||||
},
|
||||
));
|
||||
let mut config = (*turn.config).clone();
|
||||
config.model = Some("stale-parent-model".to_string());
|
||||
config.model_context_window = Some(128_000);
|
||||
config.model_auto_compact_token_limit = Some(100_000);
|
||||
Arc::get_mut(&mut turn)
|
||||
.expect("turn should be unique")
|
||||
.config = Arc::new(config);
|
||||
|
||||
let guardian_config = guardian_review_session_config(session.as_ref(), turn.as_ref())
|
||||
.await
|
||||
.expect("guardian config")
|
||||
.spawn_config;
|
||||
|
||||
assert_eq!(
|
||||
(
|
||||
guardian_config.model_context_window,
|
||||
guardian_config.model_auto_compact_token_limit,
|
||||
),
|
||||
(Some(128_000), Some(100_000))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn guardian_review_session_config_clears_parent_developer_instructions() {
|
||||
let mut parent_config = test_config().await;
|
||||
|
||||
@@ -9,6 +9,8 @@ use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::RolloutLine;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::fs_wait;
|
||||
@@ -69,6 +71,16 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review() -> Result<()
|
||||
]],
|
||||
vec![vec![
|
||||
ev_response_created("guardian-review"),
|
||||
ev_assistant_message(
|
||||
"guardian-assessment",
|
||||
&json!({
|
||||
"risk_level": "low",
|
||||
"user_authorization": "high",
|
||||
"outcome": "allow",
|
||||
"rationale": "The command is safe to execute.",
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("guardian-review"),
|
||||
]],
|
||||
])
|
||||
@@ -77,6 +89,8 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review() -> Result<()
|
||||
config.model_catalog = Some(ModelsResponse {
|
||||
models: vec![review_model],
|
||||
});
|
||||
config.model_context_window = Some(900_000);
|
||||
config.model_auto_compact_token_limit = Some(600_000);
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
});
|
||||
@@ -133,7 +147,23 @@ async fn guardian_session_prewarms_and_is_reused_for_first_review() -> Result<()
|
||||
);
|
||||
assert_eq!(guardian_review.get("generate"), None);
|
||||
|
||||
let guardian_rollout_path = test
|
||||
.codex
|
||||
.guardian_trunk_rollout_path()
|
||||
.await
|
||||
.expect("guardian trunk rollout path");
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
let guardian_context_windows = fs::read_to_string(guardian_rollout_path)?
|
||||
.lines()
|
||||
.map(serde_json::from_str::<RolloutLine>)
|
||||
.collect::<serde_json::Result<Vec<_>>>()?
|
||||
.into_iter()
|
||||
.filter_map(|line| match line.item {
|
||||
RolloutItem::EventMsg(EventMsg::TurnStarted(event)) => Some(event.model_context_window),
|
||||
_ => None,
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(guardian_context_windows, vec![Some(258_400)]);
|
||||
server.shutdown().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user