mirror of
https://github.com/openai/codex.git
synced 2026-11-07 08:34:29 +00:00
Enforce complete request budgets for Guardian reviews (#44281)
## Why Review evidence can fit its own limits while exceeding the reviewer context window once history, tools, output format, and reminders are included. Tool continuations can also push an existing review over budget. ## What changed - Select evidence after tools and turn context are resolved, reserving request overhead and a 256-token margin. Omit optional evidence while preserving required sections, and reject required evidence that cannot fit. - Keep incoming evidence pending until budget selection succeeds so cancellation and compaction cannot record unselected input in reusable history. - Check the assembled prompt before sampling, including retries and tool continuations. Retry budget failures with summary compaction once per model step, and retire reviewers whose budgets remain exhausted. Avoid history-resetting token-budget compaction for this recovery. - Record section costs after budget selection. ## Testing Add unit and integration coverage for required-evidence rejection, optional-evidence omission, reminder overhead, cancellation safety, incoming-review and tool-continuation compaction, and reviewer retirement after ineffective compaction. GitOrigin-RevId: 2f7ef191b3f46032d295ad363eb7debf9d7a9684
This commit is contained in:
1 parent
9caddc5cf5
commit
fcd90d8f07
12 files changed
+879
-70
No files matched your search
@@ -4795,8 +4795,10 @@ async fn code_mode_node_repl_text_evidence_is_visible_only_to_guardian(
|
||||
reviewer.input_modalities =
|
||||
vec![codex_protocol::openai_models::InputModality::Text];
|
||||
} else {
|
||||
// Fit the required text and request-only prefix, while leaving
|
||||
// insufficient room for the additional image reservation.
|
||||
reviewer.context_window =
|
||||
(reviewer_constraint == Some("small")).then_some(10_000);
|
||||
(reviewer_constraint == Some("small")).then_some(20_000);
|
||||
reviewer.max_context_window = reviewer.context_window;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -0,0 +1,327 @@
|
||||
//! Review requests preserve required evidence while fitting the aggregate budget.
|
||||
//! Parent token-budget mode must not replace Guardian's summary compaction.
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_core::config::CurrentTimeReminderConfig;
|
||||
use codex_core::config::RolloutBudgetConfig;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::openai_models::AutoReviewMessages;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
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_custom_tool_call;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::skip_if_wine_exec;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use test_case::test_case;
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum ReviewerResponse {
|
||||
Decision,
|
||||
ToolContinuation,
|
||||
UncompactableContinuation,
|
||||
NextReview,
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[test_case(1, ReviewerResponse::Decision; "required_context_fails_closed")]
|
||||
#[test_case(4_500, ReviewerResponse::ToolContinuation; "oversized_tool_continuation_compacts")]
|
||||
#[test_case(4_500, ReviewerResponse::UncompactableContinuation; "ineffective_compaction_fails_closed")]
|
||||
#[test_case(4_500, ReviewerResponse::NextReview; "incoming_review_compacts_existing_history")]
|
||||
async fn review_respects_complete_context_budget(
|
||||
window: i64,
|
||||
reviewer_response: ReviewerResponse,
|
||||
) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_wine_exec!(
|
||||
Ok(()),
|
||||
"Guardian approval actions require host-native paths"
|
||||
);
|
||||
let server = responses::start_mock_server().await;
|
||||
let mut builder = test_codex()
|
||||
.with_model_info_override("gpt-5.6-luna", move |model| {
|
||||
model.context_window = Some(window);
|
||||
model
|
||||
.model_messages
|
||||
.as_mut()
|
||||
.expect("bundled reviewer model messages")
|
||||
.auto_review = Some(AutoReviewMessages {
|
||||
policy: Some("Review actions against user authorization.".to_owned()),
|
||||
policy_template: Some("{{ tenant_policy_config }}".to_owned()),
|
||||
node_repl_policy: None,
|
||||
rejection_instructions: None,
|
||||
timeout_instructions: None,
|
||||
});
|
||||
})
|
||||
.with_model_info_override("gpt-5.5", |model| {
|
||||
model.auto_review_model_override = Some("gpt-5.6-luna".to_owned());
|
||||
})
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::TokenBudget)
|
||||
.expect("parent uses token-budget mode while Guardian uses summary compaction");
|
||||
config
|
||||
.features
|
||||
.disable(Feature::RemoteCompactionV2)
|
||||
.expect("test uses the legacy compact endpoint");
|
||||
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CurrentTimeReminder)
|
||||
.expect("test permits time reminders");
|
||||
config.current_time_reminder = Some(CurrentTimeReminderConfig::default());
|
||||
config.rollout_budget = Some(RolloutBudgetConfig {
|
||||
limit_tokens: 1_000_000,
|
||||
reminder_at_remaining_tokens: Vec::new(),
|
||||
sampling_token_weight: 1.0,
|
||||
prefill_token_weight: 1.0,
|
||||
});
|
||||
});
|
||||
if matches!(
|
||||
reviewer_response,
|
||||
ReviewerResponse::ToolContinuation | ReviewerResponse::UncompactableContinuation
|
||||
) {
|
||||
builder = builder
|
||||
.with_code_mode_host_program(codex_utils_cargo_bin::cargo_bin("codex-code-mode-host")?);
|
||||
}
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
let command = json!({"cmd": "echo guardian-budget-test", "sandbox_permissions": "require_escalated", "justification": "Run the requested command."}).to_string();
|
||||
let mut commentary =
|
||||
ev_assistant_message("commentary", &"optional old commentary ".repeat(/*n*/ 900));
|
||||
commentary["item"]["phase"] = json!("commentary");
|
||||
let mut events = vec![
|
||||
sse(vec![
|
||||
ev_response_created("parent-action"),
|
||||
commentary,
|
||||
ev_function_call("exec-over-budget", "exec_command", &command),
|
||||
ev_completed("parent-action"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("parent-done"),
|
||||
ev_assistant_message("done", "done"),
|
||||
ev_completed("parent-done"),
|
||||
]),
|
||||
];
|
||||
if window > 1 {
|
||||
events.insert(
|
||||
/*index*/ 1,
|
||||
sse(vec![
|
||||
ev_response_created("review"),
|
||||
match reviewer_response {
|
||||
ReviewerResponse::Decision | ReviewerResponse::NextReview => {
|
||||
ev_assistant_message(
|
||||
"decision",
|
||||
r#"{"risk_level":"low","user_authorization":"high","outcome":"allow"}"#,
|
||||
)
|
||||
}
|
||||
ReviewerResponse::ToolContinuation
|
||||
| ReviewerResponse::UncompactableContinuation => ev_custom_tool_call(
|
||||
"reviewer-inspect",
|
||||
"exec",
|
||||
"text('inspection-output'.repeat(600));",
|
||||
),
|
||||
},
|
||||
ev_completed("review"),
|
||||
]),
|
||||
);
|
||||
}
|
||||
if matches!(reviewer_response, ReviewerResponse::ToolContinuation) {
|
||||
events.insert(
|
||||
/*index*/ 2,
|
||||
sse(vec![
|
||||
ev_assistant_message(
|
||||
"after-compaction",
|
||||
r#"{"risk_level":"low","user_authorization":"high","outcome":"allow"}"#,
|
||||
),
|
||||
ev_completed("after-compaction"),
|
||||
]),
|
||||
);
|
||||
}
|
||||
if matches!(
|
||||
reviewer_response,
|
||||
ReviewerResponse::UncompactableContinuation | ReviewerResponse::NextReview
|
||||
) {
|
||||
events.extend([
|
||||
sse(vec![
|
||||
ev_function_call("retry-command", "exec_command", &command),
|
||||
ev_completed("retry-action"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_assistant_message(
|
||||
"retry-decision",
|
||||
r#"{"risk_level":"low","user_authorization":"high","outcome":"allow"}"#,
|
||||
),
|
||||
ev_completed("retry-review"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_assistant_message("retry-done", "done"),
|
||||
ev_completed("retry-done"),
|
||||
]),
|
||||
]);
|
||||
}
|
||||
let compact = if matches!(reviewer_response, ReviewerResponse::Decision) {
|
||||
None
|
||||
} else {
|
||||
let summary = if matches!(
|
||||
reviewer_response,
|
||||
ReviewerResponse::UncompactableContinuation
|
||||
) {
|
||||
"still oversized ".repeat(/*n*/ 4_000)
|
||||
} else {
|
||||
"Previous review evidence and inspection results.".to_owned()
|
||||
};
|
||||
Some(
|
||||
responses::mount_compact_json_once(
|
||||
&server,
|
||||
json!({"output": [{
|
||||
"type": "compaction", "encrypted_content": summary
|
||||
}]}),
|
||||
)
|
||||
.await,
|
||||
)
|
||||
};
|
||||
let responses = responses::mount_sse_sequence(&server, events).await;
|
||||
test.submit_text_turn("Run the command if the approval reviewer allows it.")
|
||||
.await?;
|
||||
let requests = responses.requests();
|
||||
let guardian_requests = requests
|
||||
.iter()
|
||||
.filter(|request| {
|
||||
request.body_json()["client_metadata"]["x-openai-subagent"].as_str() == Some("guardian")
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
if window == 1 {
|
||||
assert_eq!(requests.len(), 2);
|
||||
assert!(guardian_requests.is_empty());
|
||||
let output = requests[1].function_call_output("exec-over-budget");
|
||||
assert!(
|
||||
output
|
||||
.to_string()
|
||||
.contains("evidence exceeds limits for section request_budget"),
|
||||
"expected required-evidence budget rejection: {output}"
|
||||
);
|
||||
} else {
|
||||
let recovered = matches!(reviewer_response, ReviewerResponse::ToolContinuation);
|
||||
assert_eq!(requests.len(), if recovered { 4 } else { 3 });
|
||||
assert_eq!(guardian_requests.len(), if recovered { 2 } else { 1 });
|
||||
if recovered {
|
||||
let compact = compact
|
||||
.as_ref()
|
||||
.expect("scenario includes compaction")
|
||||
.single_request();
|
||||
assert!(
|
||||
compact
|
||||
.body_json()
|
||||
.to_string()
|
||||
.contains("inspection-output")
|
||||
);
|
||||
let recovered = guardian_requests[1].body_json();
|
||||
assert!(
|
||||
recovered["input"]
|
||||
.to_string()
|
||||
.contains("Previous review evidence")
|
||||
);
|
||||
assert_eq!(
|
||||
guardian_requests[0].body_json()["client_metadata"]["thread_id"],
|
||||
recovered["client_metadata"]["thread_id"]
|
||||
);
|
||||
assert!(
|
||||
requests
|
||||
.last()
|
||||
.expect("parent resumes after review")
|
||||
.function_call_output("exec-over-budget")
|
||||
.to_string()
|
||||
.contains("guardian-budget-test")
|
||||
);
|
||||
}
|
||||
if matches!(
|
||||
reviewer_response,
|
||||
ReviewerResponse::UncompactableContinuation
|
||||
) {
|
||||
let output = requests
|
||||
.last()
|
||||
.expect("parent continues after the rejected review")
|
||||
.function_call_output("exec-over-budget");
|
||||
assert!(
|
||||
output
|
||||
.to_string()
|
||||
.contains("Codex ran out of room in the model's context window"),
|
||||
"expected context-budget rejection: {output}"
|
||||
);
|
||||
}
|
||||
let request = guardian_requests[0];
|
||||
let context = request.message_input_texts("user").join("\n");
|
||||
assert!(context.contains("<guardian_context_omission>"));
|
||||
assert!(!context.contains("optional old commentary"));
|
||||
assert!(context.contains("Run the command if the approval reviewer allows it."));
|
||||
assert!(context.contains("echo guardian-budget-test"));
|
||||
let developer_context = request.message_input_texts("developer").join("\n");
|
||||
assert!(developer_context.contains("<current_time_reminder>"));
|
||||
assert!(developer_context.contains("<rollout_budget>"));
|
||||
if matches!(
|
||||
reviewer_response,
|
||||
ReviewerResponse::UncompactableContinuation | ReviewerResponse::NextReview
|
||||
) {
|
||||
let instruction = if matches!(reviewer_response, ReviewerResponse::NextReview) {
|
||||
"New instructions: retry the requested command and keep all files private. "
|
||||
.repeat(/*n*/ 12)
|
||||
} else {
|
||||
"Retry the requested command.".to_owned()
|
||||
};
|
||||
test.submit_text_turn(&instruction).await?;
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 6);
|
||||
let compact = compact
|
||||
.as_ref()
|
||||
.map(responses::ResponseMock::single_request);
|
||||
if matches!(reviewer_response, ReviewerResponse::NextReview) {
|
||||
let compact = compact.expect("scenario includes compaction");
|
||||
assert!(
|
||||
!compact
|
||||
.body_json()
|
||||
.to_string()
|
||||
.contains("New instructions:"),
|
||||
"incoming evidence must remain pending during compaction"
|
||||
);
|
||||
assert!(
|
||||
requests[4].body_json()["input"]
|
||||
.to_string()
|
||||
.contains("New instructions:")
|
||||
);
|
||||
assert!(
|
||||
requests[4].body_json()["input"]
|
||||
.to_string()
|
||||
.contains("Previous review evidence")
|
||||
);
|
||||
assert_eq!(
|
||||
request.body_json()["client_metadata"]["thread_id"],
|
||||
requests[4].body_json()["client_metadata"]["thread_id"]
|
||||
);
|
||||
} else {
|
||||
assert_ne!(
|
||||
request.body_json()["client_metadata"]["thread_id"],
|
||||
requests[4].body_json()["client_metadata"]["thread_id"],
|
||||
"failed compaction must retire the reviewer"
|
||||
);
|
||||
}
|
||||
assert!(
|
||||
requests[5]
|
||||
.function_call_output("retry-command")
|
||||
.to_string()
|
||||
.contains("guardian-budget-test")
|
||||
);
|
||||
}
|
||||
}
|
||||
test.codex.shutdown_and_wait().await?;
|
||||
Ok(())
|
||||
}
|
||||
@@ -71,6 +71,9 @@ mod external_auth;
|
||||
mod fork_thread;
|
||||
mod git_enrichment;
|
||||
mod guardian_authorization;
|
||||
// Uses the same command-approval harness as guardian_review below.
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
mod guardian_context_budget;
|
||||
mod guardian_history;
|
||||
mod guardian_mcp_elicitation;
|
||||
mod guardian_retained_context;
|
||||
|
||||
Reference in new issue
Block a user