mirror of
https://github.com/openai/codex.git
synced 2026-09-11 20:36:49 +00:00
Keep Guardian reviewers on summary-based compaction (#43912)
## Why Guardian reviewers can inherit token-budget mode from the parent session or model defaults, causing context rollover instead of summary-based compaction. ## What changed Clear inherited token-budget startup activation, set an explicit default `TokenBudgetConfig`, and disable `Feature::TokenBudget` and `Feature::ContextManagement` for Guardian review sessions. ## Testing Update regression coverage to verify that reviewers compact with a summary even when the parent and model enable token budgets. Cover both transcript modes and browser evidence, checking that review requests omit `token_budget.context_window` and retain the compaction summary. GitOrigin-RevId: e1a84f9c7752a712399cd749c3834b2a7bb4a7e6
This commit is contained in:
@@ -13,6 +13,7 @@ use tracing::warn;
|
||||
use crate::config::Config;
|
||||
use crate::config::Constrained;
|
||||
use crate::config::NetworkProxySpec;
|
||||
use crate::config::TokenBudgetConfig;
|
||||
|
||||
use super::prompt::BUNDLED_GUARDIAN_POLICY_TEMPLATE;
|
||||
use super::prompt::guardian_policy_prompt_with_config_and_template;
|
||||
@@ -43,6 +44,10 @@ pub fn build_guardian_review_session_config(
|
||||
guardian_config.include_skill_instructions = false;
|
||||
guardian_config.memories.use_memories = false;
|
||||
guardian_config.memories.dedicated_tools = false;
|
||||
// Clear inherited startup activation and keep an explicit configuration so model
|
||||
// defaults cannot re-enable token-budget mode after the feature is disabled below.
|
||||
guardian_config.token_budget_startup_config = None;
|
||||
guardian_config.token_budget = Some(TokenBudgetConfig::default());
|
||||
let catalog_auto_review = model_messages.and_then(|messages| messages.auto_review.as_ref());
|
||||
let tenant_policy_config = parent_config.resolve_guardian_policy(model_messages);
|
||||
let policy_template = catalog_auto_review
|
||||
@@ -90,6 +95,8 @@ pub fn build_guardian_review_session_config(
|
||||
Feature::Collab,
|
||||
Feature::MultiAgentV2,
|
||||
Feature::GuardianV2,
|
||||
Feature::TokenBudget,
|
||||
Feature::ContextManagement,
|
||||
Feature::CodexHooks,
|
||||
Feature::Apps,
|
||||
Feature::Plugins,
|
||||
|
||||
@@ -4707,8 +4707,7 @@ await tools.exec_command({ cmd: "true", sandbox_permissions: "require_escalated"
|
||||
#[test_case("node_repl", true, true, false, Some("unbounded"); "text_fallback_without_context_bound")]
|
||||
#[test_case("node_repl", true, true, false, Some("small"); "text_fallback_with_insufficient_context")]
|
||||
#[test_case("node_repl", true, true, false, Some("large_prompt"); "images_resume_after_prompt_pressure")]
|
||||
#[test_case("node_repl", true, true, false, Some("buffered"); "multimodal_reviewer_preserves_model_owned_fallback_buffer")]
|
||||
#[test_case("node_repl", true, true, false, Some("rollover"); "multimodal_reviewer_rollover_replays_evidence")]
|
||||
#[test_case("node_repl", true, true, false, Some("compaction"); "multimodal_reviewer_compaction_preserves_evidence")]
|
||||
#[test_case("cua_repl", false, false, false, None; "cua_disabled")]
|
||||
#[test_case("cua_repl", true, false, false, None; "cua_manually_enabled_text_only")]
|
||||
#[test_case("cua_repl", true, true, false, None; "cua_manually_enabled_multimodal")]
|
||||
@@ -4738,8 +4737,18 @@ async fn code_mode_node_repl_text_evidence_is_visible_only_to_guardian(
|
||||
const PRIVATE_IMAGE: &str = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAADUlEQVR4nGP4z8DwHwAFAAH/iZk9HQAAAABJRU5ErkJggg==";
|
||||
let server = responses::start_mock_server().await;
|
||||
let mcp_server_bin = remote_aware_stdio_server_bin()?;
|
||||
let reviewer_rollover = reviewer_constraint == Some("rollover");
|
||||
let reviewer_token_budget = matches!(reviewer_constraint, Some("buffered" | "rollover"));
|
||||
let reviewer_compaction = reviewer_constraint == Some("compaction");
|
||||
let compact = if reviewer_compaction {
|
||||
Some(
|
||||
responses::mount_compact_user_history_with_summary_once(
|
||||
&server,
|
||||
"Guardian browser evidence summary",
|
||||
)
|
||||
.await,
|
||||
)
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let check_detail = enhanced_transcripts && transcript_images && reviewer_constraint.is_none();
|
||||
let mut large_image = Cursor::new(Vec::new());
|
||||
if check_detail {
|
||||
@@ -4762,7 +4771,7 @@ async fn code_mode_node_repl_text_evidence_is_visible_only_to_guardian(
|
||||
.iter_mut()
|
||||
.find(|model| model.slug == "gpt-5.6-luna")
|
||||
.expect("API-key Guardian reviewer");
|
||||
if reviewer_token_budget {
|
||||
if reviewer_compaction {
|
||||
reviewer.context_window = Some(100_000);
|
||||
reviewer.max_context_window = Some(100_000);
|
||||
reviewer.auto_compact_token_limit = Some(50_000);
|
||||
@@ -4781,7 +4790,8 @@ async fn code_mode_node_repl_text_evidence_is_visible_only_to_guardian(
|
||||
.features
|
||||
.enable(Feature::CodeMode)
|
||||
.expect("enable Code Mode");
|
||||
if reviewer_token_budget {
|
||||
if reviewer_compaction {
|
||||
config.features.disable(Feature::RemoteCompactionV2).expect("use remote compaction");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::TokenBudget)
|
||||
@@ -4823,8 +4833,7 @@ async fn code_mode_node_repl_text_evidence_is_visible_only_to_guardian(
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
wait_for_mcp_server(&test.codex, repl_server).await?;
|
||||
let images_enabled = auto_review_required || (enhanced_transcripts && transcript_images);
|
||||
let reviewer_images =
|
||||
images_enabled && (reviewer_constraint.is_none() || reviewer_token_budget);
|
||||
let reviewer_images = images_enabled && (reviewer_constraint.is_none() || reviewer_compaction);
|
||||
let snapshot_padding = if images_enabled && reviewer_constraint != Some("large_prompt") {
|
||||
2_500
|
||||
} else {
|
||||
@@ -4853,13 +4862,13 @@ await tools.mcp__node_repl__js({ code: 'nodeRepl.empty()' });
|
||||
await tools.mcp__node_repl__js({ code: 'await nodeRepl.emitImage(await tab.screenshot())' });
|
||||
if (LARGE_IMAGE) await tools.mcp__node_repl__image_scenario({ scenario: "invalid_image_bytes_then_image" });
|
||||
await tools.exec_command({ cmd: "true", sandbox_permissions: "require_escalated", justification: "review" });
|
||||
if (!REVIEWER_ROLLOVER) await tools.mcp__node_repl__js({ code: 'await nodeRepl.emitImage(await tab.screenshot())' });
|
||||
if (!REVIEWER_COMPACTION) await tools.mcp__node_repl__js({ code: 'await nodeRepl.emitImage(await tab.screenshot())' });
|
||||
if (LARGE_IMAGE) await tools.mcp__node_repl__image({});
|
||||
await tools.exec_command({ cmd: "printf second", sandbox_permissions: "require_escalated", justification: "review again" });
|
||||
"#
|
||||
.replace("node_repl", repl_server)
|
||||
.replace("SNAPSHOT_PADDING", &snapshot_padding.to_string())
|
||||
.replace("REVIEWER_ROLLOVER", &reviewer_rollover.to_string())
|
||||
.replace("REVIEWER_COMPACTION", &reviewer_compaction.to_string())
|
||||
.replace("LARGE_IMAGE", &check_detail.to_string());
|
||||
let response_mock = responses::mount_sse_sequence(
|
||||
&server,
|
||||
@@ -4888,10 +4897,10 @@ await tools.exec_command({ cmd: "printf second", sandbox_permissions: "require_e
|
||||
]),
|
||||
sse(vec![
|
||||
ev_assistant_message("guardian", r#"{"outcome":"allow"}"#),
|
||||
if reviewer_token_budget {
|
||||
if reviewer_compaction {
|
||||
responses::ev_completed_with_tokens(
|
||||
"resp-guardian",
|
||||
/*total_tokens*/ if reviewer_rollover { 70_000 } else { 60_000 },
|
||||
/*total_tokens*/ 70_000,
|
||||
)
|
||||
} else {
|
||||
ev_completed("resp-guardian")
|
||||
@@ -4996,43 +5005,21 @@ await tools.exec_command({ cmd: "printf second", sandbox_permissions: "require_e
|
||||
reviewer_image_urls
|
||||
}
|
||||
);
|
||||
if reviewer_rollover {
|
||||
let second_request = guardian_requests[1];
|
||||
let second_prompt = second_request
|
||||
.message_input_text_groups("user")
|
||||
.last()
|
||||
.expect("post-rollover Guardian review prompt")
|
||||
.join("");
|
||||
assert!(second_prompt.contains(">>> TRANSCRIPT START\n"));
|
||||
assert!(!second_prompt.contains(">>> TRANSCRIPT DELTA START\n"));
|
||||
assert!(second_prompt.contains(NODE_REPL_DOM_MIDDLE));
|
||||
if let Some(compact) = compact {
|
||||
let compact_request = compact.single_request();
|
||||
assert!(
|
||||
compact_request
|
||||
.message_input_texts("user")
|
||||
.concat()
|
||||
.contains(NODE_REPL_DOM_MIDDLE)
|
||||
);
|
||||
assert_eq!(
|
||||
second_request.message_input_image_urls("user"),
|
||||
vec![PRIVATE_IMAGE.to_string()],
|
||||
"browser screenshots must be replayed into the fresh reviewer window"
|
||||
);
|
||||
assert!(
|
||||
second_request
|
||||
.message_input_texts("developer")
|
||||
.iter()
|
||||
.any(|text| text.contains("Previous context window id:")),
|
||||
"Guardian should roll over before rebuilding browser evidence"
|
||||
);
|
||||
} else if reviewer_token_budget {
|
||||
let second_request = guardian_requests[1];
|
||||
let second_prompt = second_request
|
||||
.message_input_text_groups("user")
|
||||
.last()
|
||||
.expect("buffered Guardian review prompt")
|
||||
.join("");
|
||||
assert!(second_prompt.contains(">>> TRANSCRIPT DELTA START\n"));
|
||||
assert!(
|
||||
second_request
|
||||
.message_input_texts("developer")
|
||||
.iter()
|
||||
.all(|text| !text.contains("Previous context window id:")),
|
||||
"Guardian should preserve the reviewer model's fallback buffer before rolling over"
|
||||
guardian_requests[1].inputs_of_type("compaction")[0]["encrypted_content"],
|
||||
"Guardian browser evidence summary"
|
||||
);
|
||||
for request in &guardian_requests {
|
||||
assert!(!request.has_content_kinds(&["token_budget.context_window"]));
|
||||
}
|
||||
}
|
||||
let parent_request = requests.last().expect("parent turn should complete");
|
||||
let parent_input = serde_json::to_string(&parent_request.input())?;
|
||||
|
||||
@@ -32,6 +32,7 @@ use codex_protocol::models::PermissionProfileSnapshot;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::openai_models::AutoReviewMessages;
|
||||
use codex_protocol::openai_models::MODEL_SPECIALTY_CYBER;
|
||||
use codex_protocol::openai_models::ModelTokenBudgetConfig;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
@@ -288,10 +289,9 @@ async fn guardian_session_inherits_parent_http_fallback(
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[test_case(60_000, false; "legacy_reviewer_window_is_already_exhausted")]
|
||||
#[test_case(49_990, true; "retained_followup_reminder_exhausts_reviewer_window")]
|
||||
async fn guardian_review_resends_full_transcript_after_reviewer_context_rollover(
|
||||
first_review_total_tokens: i64,
|
||||
#[test_case(false; "legacy_transcript")]
|
||||
#[test_case(true; "thread_owned_transcript")]
|
||||
async fn guardian_review_compacts_with_summary_despite_parent_token_budget(
|
||||
thread_owned: bool,
|
||||
) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
@@ -301,15 +301,37 @@ async fn guardian_review_resends_full_transcript_after_reviewer_context_rollover
|
||||
);
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let summary = "Guardian retained the user's standing authorization.";
|
||||
let compact = core_test_support::responses::mount_compact_user_history_with_summary_once(
|
||||
&server, summary,
|
||||
)
|
||||
.await;
|
||||
let mut builder = test_codex()
|
||||
.with_model_info_override("gpt-5.5", |model| {
|
||||
model.auto_review_model_override = Some(model.slug.clone());
|
||||
model
|
||||
.model_messages
|
||||
.as_mut()
|
||||
.expect("model messages")
|
||||
.token_budget = Some(ModelTokenBudgetConfig {
|
||||
enabled: true,
|
||||
use_history_notes_extension: true,
|
||||
reminder_threshold_tokens: 6_144,
|
||||
reminder_message_template: "{n_remaining} tokens remain.".to_string(),
|
||||
guidance_message: "Save state before resetting context.".to_string(),
|
||||
auto_compact_fallback_prompt: "Save important state.".to_string(),
|
||||
auto_compact_fallback_buffer_tokens: 16_384,
|
||||
});
|
||||
})
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.set_enabled(Feature::GuardianThreadContext, thread_owned)
|
||||
.expect("configure Guardian context mode");
|
||||
config
|
||||
.features
|
||||
.disable(Feature::RemoteCompactionV2)
|
||||
.expect("use remote compaction");
|
||||
config.model_context_window = Some(100_000);
|
||||
config.model_auto_compact_token_limit = Some(50_000);
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
@@ -338,7 +360,7 @@ async fn guardian_review_resends_full_transcript_after_reviewer_context_rollover
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian-first"),
|
||||
ev_assistant_message("guardian-first", approval),
|
||||
ev_completed_with_tokens("resp-guardian-first", first_review_total_tokens),
|
||||
ev_completed_with_tokens("resp-guardian-first", /*total_tokens*/ 60_000),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-second"),
|
||||
@@ -373,31 +395,37 @@ async fn guardian_review_resends_full_transcript_after_reviewer_context_rollover
|
||||
assert_eq!(
|
||||
guardian_requests[0].body_json()["client_metadata"]["thread_id"],
|
||||
guardian_requests[1].body_json()["client_metadata"]["thread_id"],
|
||||
"the same Guardian reviewer should survive the context-window rollover"
|
||||
"the same Guardian reviewer should survive compaction"
|
||||
);
|
||||
|
||||
let second_request = guardian_requests[1];
|
||||
let second_prompt = second_request
|
||||
.message_input_text_groups("user")
|
||||
.last()
|
||||
.expect("post-rollover Guardian review prompt")
|
||||
.join("");
|
||||
assert!(requests[0].has_content_kinds(&["token_budget.context_window"]));
|
||||
for request in &guardian_requests {
|
||||
assert!(!request.has_content_kinds(&["token_budget.context_window"]));
|
||||
}
|
||||
let compact_request = compact.single_request();
|
||||
assert!(
|
||||
second_request
|
||||
.message_input_texts("developer")
|
||||
.iter()
|
||||
.any(|text| text.contains("Previous context window id:")),
|
||||
"Guardian should have rolled into a new context window"
|
||||
compact_request
|
||||
.message_input_texts("user")
|
||||
.join("\n")
|
||||
.contains(user_authorization)
|
||||
);
|
||||
let second_request = guardian_requests[1];
|
||||
assert_eq!(
|
||||
second_request.inputs_of_type("compaction")[0]["encrypted_content"],
|
||||
summary
|
||||
);
|
||||
assert!(second_prompt.contains(">>> TRANSCRIPT START\n"));
|
||||
assert!(!second_prompt.contains(">>> TRANSCRIPT DELTA START\n"));
|
||||
assert!(second_prompt.contains(user_authorization));
|
||||
assert!(
|
||||
second_request
|
||||
.message_input_texts("user")
|
||||
.join("\n")
|
||||
.contains(user_authorization)
|
||||
);
|
||||
assert!(
|
||||
compact_request
|
||||
.message_input_texts("developer")
|
||||
.iter()
|
||||
.any(|text| text.contains("Use prior reviews as context, not binding precedent.")),
|
||||
"the follow-up policy reminder should survive reviewer context rollover"
|
||||
"the compactor should receive the follow-up policy reminder"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
|
||||
Reference in New Issue
Block a user