mirror of
https://github.com/openai/codex.git
synced 2026-11-10 21:00:47 +00:00
Preserve complete actions in Guardian approval reviews (#44569)
## Why Truncating action arguments can leave approval reviewers evaluating incomplete actions. Large actions need complete review input and explicit handling when they exceed the review budget. ## What changed - Remove action truncation and the fixed synchronous action byte limit. Admit complete actions against the whole-request budget, splitting long text losslessly into bounded transport parts and accounting for their framing. - Route actions exceeding the asynchronous action budget to synchronous review. Prevent cached scores from covering oversized calls, including expanded approval arguments, while allowing later small actions to recover score reuse. - Request user approval when optional review exhausts its local input budget. Keep required review and compaction service failures closed to approval, and retire exhausted review sessions. ## Testing Add coverage for complete large-action delivery, optional user fallback, required-review denial, subsequent review recovery, async overflow through MCP approval routing, and lossless text splitting with budget accounting. GitOrigin-RevId: 08f06e68a94b2f779480dd6b6cf5bc30b241f6dd
This commit is contained in:
@@ -12,13 +12,8 @@ use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use serde::Serialize;
|
||||
use serde::ser::Error as _;
|
||||
use serde_json::Value;
|
||||
|
||||
use super::GUARDIAN_MAX_ACTION_BYTES;
|
||||
use super::GUARDIAN_MAX_ACTION_STRING_TOKENS;
|
||||
use super::prompt::guardian_truncate_text;
|
||||
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
pub(crate) enum GuardianApprovalRequest {
|
||||
ExecCommand {
|
||||
@@ -249,49 +244,6 @@ fn guardian_command_source_tool_name(source: GuardianCommandSource) -> &'static
|
||||
}
|
||||
}
|
||||
|
||||
fn truncate_guardian_action_value(value: Value) -> (Value, bool) {
|
||||
match value {
|
||||
Value::String(text) => {
|
||||
let (text, truncated) =
|
||||
guardian_truncate_text(&text, GUARDIAN_MAX_ACTION_STRING_TOKENS);
|
||||
(Value::String(text), truncated)
|
||||
}
|
||||
Value::Array(values) => {
|
||||
let mut truncated = false;
|
||||
let values = values
|
||||
.into_iter()
|
||||
.map(|value| {
|
||||
let (value, value_truncated) = truncate_guardian_action_value(value);
|
||||
truncated |= value_truncated;
|
||||
value
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
(Value::Array(values), truncated)
|
||||
}
|
||||
Value::Object(values) => {
|
||||
let mut entries = values.into_iter().collect::<Vec<_>>();
|
||||
entries.sort_by(|(left, _), (right, _)| left.cmp(right));
|
||||
let mut truncated = false;
|
||||
let values = entries
|
||||
.into_iter()
|
||||
.map(|(key, value)| {
|
||||
let (value, value_truncated) = truncate_guardian_action_value(value);
|
||||
truncated |= value_truncated;
|
||||
(key, value)
|
||||
})
|
||||
.collect();
|
||||
(Value::Object(values), truncated)
|
||||
}
|
||||
other => (other, false),
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub(crate) struct FormattedGuardianAction {
|
||||
pub(crate) text: String,
|
||||
pub(crate) truncated: bool,
|
||||
}
|
||||
|
||||
pub(crate) fn guardian_approval_request_to_json(
|
||||
action: &GuardianApprovalRequest,
|
||||
) -> serde_json::Result<Value> {
|
||||
@@ -584,26 +536,10 @@ pub(crate) fn guardian_request_turn_id<'a>(
|
||||
|
||||
pub(crate) fn format_guardian_action_pretty(
|
||||
action: &GuardianApprovalRequest,
|
||||
) -> serde_json::Result<FormattedGuardianAction> {
|
||||
let value = guardian_action_for_review(action)?;
|
||||
let (value, truncated) = truncate_guardian_action_value(value);
|
||||
let text = enforce_guardian_action_byte_limit(serde_json::to_string_pretty(&value)?)?;
|
||||
Ok(FormattedGuardianAction { text, truncated })
|
||||
}
|
||||
|
||||
fn enforce_guardian_action_byte_limit(text: String) -> serde_json::Result<String> {
|
||||
if text.len() > GUARDIAN_MAX_ACTION_BYTES {
|
||||
return Err(serde_json::Error::custom(format!(
|
||||
"Guardian action exceeds the {GUARDIAN_MAX_ACTION_BYTES}-byte review limit"
|
||||
)));
|
||||
}
|
||||
Ok(text)
|
||||
}
|
||||
|
||||
pub(crate) fn format_guardian_action_compact(
|
||||
action: &GuardianApprovalRequest,
|
||||
) -> serde_json::Result<String> {
|
||||
enforce_guardian_action_byte_limit(serde_json::to_string(&guardian_action_for_review(action)?)?)
|
||||
let mut value = guardian_action_for_review(action)?;
|
||||
value.sort_all_objects();
|
||||
serde_json::to_string_pretty(&value)
|
||||
}
|
||||
|
||||
fn guardian_action_for_review(action: &GuardianApprovalRequest) -> serde_json::Result<Value> {
|
||||
|
||||
@@ -79,9 +79,6 @@ pub(crate) async fn decide_approval(
|
||||
.as_ref()
|
||||
.is_some_and(CancellationToken::is_cancelled)
|
||||
|| reasons.retry.is_some()
|
||||
|| request.request.as_ref().is_ok_and(|request| {
|
||||
super::approval_request::format_guardian_action_compact(request).is_err()
|
||||
})
|
||||
|| matches!(&request.request, Ok(GuardianApprovalRequest::ExecCommand { sandbox_permissions, .. })
|
||||
if sandbox_permissions.requires_escalated_permissions());
|
||||
let full_access = context.environments().has_full_access(
|
||||
@@ -115,7 +112,10 @@ pub(crate) async fn decide_approval(
|
||||
review_id: review_id.clone(),
|
||||
request: request.clone(),
|
||||
reasons,
|
||||
options,
|
||||
options: GuardianReviewOptions {
|
||||
require_guardian,
|
||||
..options
|
||||
},
|
||||
});
|
||||
let input = ApprovalDecisionInput {
|
||||
approval_id: &review_id,
|
||||
@@ -179,9 +179,7 @@ pub(crate) async fn decide_approval(
|
||||
record_guardian_non_denial(&session, turn_id).await;
|
||||
Some(ReviewDecision::Approved)
|
||||
}
|
||||
Some(ApprovalDecision::Allow) => {
|
||||
Some(runtime.review(GuardianReviewReason::FreshRequired).await)
|
||||
}
|
||||
Some(ApprovalDecision::Allow) => runtime.review(GuardianReviewReason::FreshRequired).await,
|
||||
Some(ApprovalDecision::AskUser) if !require_guardian => None,
|
||||
None if !require_guardian
|
||||
&& !super::review::routes_approval_policy_to_guardian(
|
||||
@@ -192,7 +190,7 @@ pub(crate) async fn decide_approval(
|
||||
None
|
||||
}
|
||||
None | Some(ApprovalDecision::AskUser) => {
|
||||
Some(runtime.review(GuardianReviewReason::Policy).await)
|
||||
runtime.review(GuardianReviewReason::Policy).await
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -36,8 +36,8 @@ pub(super) async fn record_failed_review(
|
||||
target_item_id: guardian_request_target_item_id(¶ms.request),
|
||||
reviewer_thread_id: reviewer.thread_id(),
|
||||
model: ¶ms.model,
|
||||
action: &action.text,
|
||||
action_truncated: action.truncated,
|
||||
action: &action,
|
||||
action_truncated: false,
|
||||
instructions: Some(&instructions.text),
|
||||
history: history.raw_items().collect(),
|
||||
});
|
||||
|
||||
@@ -52,7 +52,13 @@ pub(crate) async fn check_pending(session: &Session, turn: &TurnContext) -> Code
|
||||
},
|
||||
GuardianBudgetOmission.render(),
|
||||
)
|
||||
.map_err(|error| CodexErr::InvalidRequest(error.to_string()))?;
|
||||
.map_err(|_error| {
|
||||
session
|
||||
.services
|
||||
.thread_extension_data
|
||||
.insert(super::request_budget::ExhaustedReviewBudget::Detected);
|
||||
CodexErr::ContextWindowExceeded
|
||||
})?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
@@ -144,7 +150,7 @@ pub(crate) async fn finalize(
|
||||
session
|
||||
.services
|
||||
.thread_extension_data
|
||||
.insert(super::request_budget::ExhaustedReviewBudget);
|
||||
.insert(super::request_budget::ExhaustedReviewBudget::Detected);
|
||||
match error {
|
||||
codex_guardian_context::SectionError::EvidenceLimitExceeded { .. } => {
|
||||
CodexErr::ContextWindowExceeded
|
||||
|
||||
@@ -69,8 +69,6 @@ const GUARDIAN_MAX_TOOL_ENTRY_TOKENS: usize = codex_guardian_context::ContextPro
|
||||
.tool_tokens;
|
||||
pub(crate) const GUARDIAN_MAX_ROOT_MESSAGE_TOKENS: usize = 900;
|
||||
pub(crate) const GUARDIAN_MAX_NODE_REPL_TOOL_RESULT_TOKENS: usize = 6_000;
|
||||
pub(crate) const GUARDIAN_MAX_ACTION_BYTES: usize = 50_000 * 4;
|
||||
const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000;
|
||||
|
||||
/// Captures review inputs from the issuing step without retaining its MCP bindings or tool router.
|
||||
/// Background network approvals and Unix interception use the active task's resolved settings.
|
||||
|
||||
@@ -42,7 +42,6 @@ pub(crate) struct GuardianPromptItems {
|
||||
pub(crate) context: ComposedContext,
|
||||
pub(crate) transcript_cursor: GuardianTranscriptCursor,
|
||||
pub(crate) node_repl_evidence_sequence: u64,
|
||||
pub(crate) reviewed_action_truncated: bool,
|
||||
}
|
||||
|
||||
/// Points to the end of the transcript that the guardian has already reviewed.
|
||||
@@ -120,7 +119,7 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn(
|
||||
.fragments;
|
||||
let planned_action_json = format_guardian_action_pretty(&request)?;
|
||||
let planned_action = PlannedAction {
|
||||
json: planned_action_json.text,
|
||||
json: planned_action_json,
|
||||
tool_descriptions: if let GuardianApprovalRequest::McpToolCall {
|
||||
tool_description,
|
||||
connector_description,
|
||||
@@ -234,7 +233,6 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn(
|
||||
context,
|
||||
transcript_cursor,
|
||||
node_repl_evidence_sequence,
|
||||
reviewed_action_truncated: planned_action_json.truncated,
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -19,7 +19,11 @@ pub(super) const INPUT_TOKEN_MARGIN: usize = 256;
|
||||
|
||||
// Keep a failed reviewer out of reuse unless compaction and a fresh budget
|
||||
// check succeed. Also distinguishes local budget failures from backend errors.
|
||||
pub(crate) struct ExhaustedReviewBudget;
|
||||
pub(crate) enum ExhaustedReviewBudget {
|
||||
Detected,
|
||||
// A compaction service failure must not be mistaken for local exhaustion.
|
||||
Compacting,
|
||||
}
|
||||
|
||||
pub(crate) fn observe(telemetry: &SessionTelemetry, request: &ResponsesApiRequest) -> usize {
|
||||
let total = estimate_request_tokens(request);
|
||||
@@ -74,7 +78,7 @@ pub(crate) fn check_prompt(
|
||||
session
|
||||
.services
|
||||
.thread_extension_data
|
||||
.insert(ExhaustedReviewBudget);
|
||||
.insert(ExhaustedReviewBudget::Detected);
|
||||
return Err(CodexErr::ContextWindowExceeded);
|
||||
}
|
||||
session
|
||||
|
||||
@@ -116,6 +116,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime {
|
||||
let completed = report.complete(
|
||||
GuardianReviewOutcome::Error(GuardianReviewError::Cancelled),
|
||||
turn.model_info(),
|
||||
self.options.require_guardian,
|
||||
GuardianReviewAnalyticsResult::without_session(),
|
||||
completed_at_ms,
|
||||
);
|
||||
@@ -150,7 +151,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime {
|
||||
format_guardian_action_pretty(&request).ok().map(|action| {
|
||||
(
|
||||
evidence,
|
||||
action.text,
|
||||
action,
|
||||
authorization_version,
|
||||
root_authorization_version,
|
||||
)
|
||||
@@ -191,7 +192,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime {
|
||||
prepared: PreparedApproval,
|
||||
mut outcome: GuardianReviewOutcome,
|
||||
analytics_result: GuardianReviewAnalyticsResult,
|
||||
) -> ReviewDecision {
|
||||
) -> Option<ReviewDecision> {
|
||||
let PreparedApproval {
|
||||
request: _,
|
||||
turn,
|
||||
@@ -225,6 +226,7 @@ impl ReviewHost for super::super::runtime::ReviewRuntime {
|
||||
let completed = report.complete(
|
||||
outcome,
|
||||
turn.model_info(),
|
||||
self.options.require_guardian,
|
||||
analytics_result,
|
||||
completed_at_ms,
|
||||
);
|
||||
|
||||
@@ -39,7 +39,6 @@ use codex_protocol::models::ResponseInputItem;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use codex_protocol::openai_models::InputModality;
|
||||
use codex_protocol::openai_models::ReasoningEffort as ReasoningEffortConfig;
|
||||
#[cfg(test)]
|
||||
use codex_protocol::protocol::CodexErrorInfo;
|
||||
use codex_protocol::protocol::Event;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
@@ -610,7 +609,6 @@ async fn run_review_on_session(
|
||||
);
|
||||
}
|
||||
};
|
||||
let reviewed_action_truncated = prompt_items.reviewed_action_truncated;
|
||||
let transcript_cursor = prompt_items.transcript_cursor;
|
||||
let node_repl_evidence_admission = (prompt_items.node_repl_evidence_sequence
|
||||
> last_admitted_node_repl_response_sequence)
|
||||
@@ -691,7 +689,6 @@ async fn run_review_on_session(
|
||||
response_sequence,
|
||||
});
|
||||
}
|
||||
analytics_result.reviewed_action_truncated = reviewed_action_truncated;
|
||||
|
||||
let outcome = wait_for_guardian_review(
|
||||
review_session,
|
||||
@@ -719,14 +716,29 @@ async fn run_review_on_session(
|
||||
state.prior_review_count = state.prior_review_count.saturating_add(1);
|
||||
state.last_reviewed_transcript_cursor = Some(transcript_cursor);
|
||||
}
|
||||
let keep_review_session = outcome.1
|
||||
&& review_session
|
||||
.session
|
||||
.services
|
||||
.thread_extension_data
|
||||
.remove::<super::request_budget::ExhaustedReviewBudget>()
|
||||
.is_none();
|
||||
(outcome.0, keep_review_session, analytics_result)
|
||||
let budget_exhausted = review_session
|
||||
.session
|
||||
.services
|
||||
.thread_extension_data
|
||||
.remove::<super::request_budget::ExhaustedReviewBudget>();
|
||||
let result = match outcome.0 {
|
||||
GuardianReviewSessionOutcome::SessionFailed {
|
||||
error_info: Some(CodexErrorInfo::ContextWindowExceeded),
|
||||
..
|
||||
} if matches!(
|
||||
budget_exhausted.as_deref(),
|
||||
Some(super::request_budget::ExhaustedReviewBudget::Detected)
|
||||
) =>
|
||||
{
|
||||
GuardianReviewSessionOutcome::InputBudgetExceeded
|
||||
}
|
||||
result => result,
|
||||
};
|
||||
(
|
||||
result,
|
||||
outcome.1 && budget_exhausted.is_none(),
|
||||
analytics_result,
|
||||
)
|
||||
}
|
||||
|
||||
async fn ensure_guardian_followup_reminder(review_session: &GuardianReviewSession) {
|
||||
|
||||
@@ -7,7 +7,6 @@ use crate::config::NetworkProxySpec;
|
||||
use crate::config::PermissionProfileSnapshot;
|
||||
use crate::config::test_config;
|
||||
use crate::environment_selection::TurnEnvironmentState;
|
||||
use crate::guardian::approval_request::format_guardian_action_compact;
|
||||
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;
|
||||
@@ -1130,46 +1129,6 @@ fn guardian_truncate_text_keeps_prefix_suffix_and_xml_marker() {
|
||||
assert!(was_truncated);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_action_formatters_reject_large_aggregate_payloads() {
|
||||
let file: PathUri = test_path_buf("/tmp/file").abs().into();
|
||||
let action = GuardianApprovalRequest::ApplyPatch {
|
||||
id: "patch-1".to_string(),
|
||||
cwd: test_path_buf("/tmp").abs().into(),
|
||||
files: vec![file; 20_000],
|
||||
patch: String::new(),
|
||||
};
|
||||
|
||||
for error in [
|
||||
format_guardian_action_pretty(&action).map(|_| ()),
|
||||
format_guardian_action_compact(&action).map(|_| ()),
|
||||
] {
|
||||
assert_eq!(
|
||||
error
|
||||
.expect_err("aggregate action should exceed the review limit")
|
||||
.to_string(),
|
||||
"Guardian action exceeds the 200000-byte review limit"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn format_guardian_action_pretty_reports_no_truncation_for_small_payload() -> serde_json::Result<()>
|
||||
{
|
||||
let action = GuardianApprovalRequest::ApplyPatch {
|
||||
id: "patch-1".to_string(),
|
||||
cwd: test_path_buf("/tmp").abs().into(),
|
||||
files: Vec::new(),
|
||||
patch: "line\n".to_string(),
|
||||
};
|
||||
|
||||
let rendered = format_guardian_action_pretty(&action)?;
|
||||
|
||||
assert!(rendered.text.contains("\"tool\": \"apply_patch\""));
|
||||
assert!(!rendered.truncated);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn guardian_approval_request_to_json_renders_mcp_tool_call_shape() -> serde_json::Result<()> {
|
||||
let action = GuardianApprovalRequest::McpToolCall {
|
||||
|
||||
@@ -333,6 +333,9 @@ pub(crate) async fn run_turn(
|
||||
}
|
||||
// Incoming evidence can overflow even below the normal history
|
||||
// threshold. Keep it pending while compacting, then select once more.
|
||||
sess.services
|
||||
.thread_extension_data
|
||||
.insert(crate::guardian::ExhaustedReviewBudget::Compacting);
|
||||
run_auto_compact(
|
||||
&sess,
|
||||
Arc::clone(&first_step_context),
|
||||
@@ -704,6 +707,9 @@ pub(crate) async fn run_turn(
|
||||
// token-budget resets must fail closed and retire the reviewer.
|
||||
// Retry once per model step, so ineffective compaction cannot loop.
|
||||
guardian_budget_compacted = true;
|
||||
sess.services
|
||||
.thread_extension_data
|
||||
.insert(crate::guardian::ExhaustedReviewBudget::Compacting);
|
||||
run_auto_compact(
|
||||
&sess,
|
||||
Arc::clone(&step_context),
|
||||
|
||||
@@ -865,11 +865,9 @@ impl UnifiedExecProcessManager {
|
||||
.map_err(|_| unreviewable_input_error())?;
|
||||
// Bound the entire serialized action plus its reason, including JSON
|
||||
// escaping. Reject, never execute an unreviewed tail.
|
||||
let oversized = reviewed.text.len().saturating_add(approval_reason.len())
|
||||
> MAX_STDIN_APPROVAL_BYTES;
|
||||
let size_check_result = if reviewed.truncated {
|
||||
"formatter_truncated"
|
||||
} else if oversized {
|
||||
let oversized =
|
||||
reviewed.len().saturating_add(approval_reason.len()) > MAX_STDIN_APPROVAL_BYTES;
|
||||
let size_check_result = if oversized {
|
||||
"over_limit"
|
||||
} else {
|
||||
"within_limit"
|
||||
@@ -884,7 +882,7 @@ impl UnifiedExecProcessManager {
|
||||
/*inc*/ 1,
|
||||
&[("result", size_check_result), ("input_kind", input_kind)],
|
||||
);
|
||||
if reviewed.truncated || oversized {
|
||||
if oversized {
|
||||
return Err(unreviewable_input_error());
|
||||
}
|
||||
let approval_context = ApprovalContext {
|
||||
|
||||
Reference in New Issue
Block a user