From 7d592f6dd9988e349e3164feaa21d0fc14bbac8a Mon Sep 17 00:00:00 2001 From: ioanbudea-OAI Date: Fri, 23 Oct 2026 13:26:41 +0000 Subject: [PATCH] Move Guardian approval reason caps into context composition (#56477) ## What changed Apply the 512-token approval reason cap in `guardian-context` when composing synchronous full and delta reviews, before request budget enforcement. Remove truncation from the requesting host and preserve exact action JSON. ## Testing Extend the planned-action budget test to cover long approval reasons in both synchronous presentations, verifying that optional tool descriptions can be omitted without changing action arguments. GitOrigin-RevId: 8cdb6aadd5f50194f63ebd2402193e69bed127cc --- codex-rs/Cargo.lock | 1 + codex-rs/core/src/guardian/prompt.rs | 10 +- codex-rs/guardian-context/Cargo.toml | 1 + codex-rs/guardian-context/src/composition.rs | 16 +++- .../guardian-context/src/enforcement_tests.rs | 93 ++++++++++++------- codex-rs/guardian-context/src/lib.rs | 2 +- 6 files changed, 76 insertions(+), 47 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index f2b0fbac81..8bdb36c9d2 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3586,6 +3586,7 @@ dependencies = [ "codex-history", "codex-protocol", "codex-protocol-test-support", + "codex-utils-output-truncation", "pretty_assertions", "serde", "serde_json", diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index e905a931dd..f82230d4ec 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -27,9 +27,7 @@ use crate::context::NodeReplReviewEvidenceMode; use crate::context::is_guardian_context_message; use crate::context::node_repl_review_evidence_mode; use crate::session::session::Session; -use codex_utils_output_truncation::TruncationPolicy; use codex_utils_output_truncation::approx_bytes_for_tokens; -use codex_utils_output_truncation::truncate_text; use super::ApprovalRequestReasons; use super::GUARDIAN_MAX_NODE_REPL_TOOL_RESULT_TOKENS; @@ -38,7 +36,6 @@ use super::GuardianApprovalRequest; use super::GuardianReviewContext; use super::approval_request::format_guardian_action_pretty; -const GUARDIAN_MAX_APPROVAL_REASON_TOKENS: usize = 512; // Bound both JSON and permission evidence without restricting manual approvals. const MAX_GUARDIAN_ENVIRONMENT_ID_BYTES: usize = 256; pub(super) const GUARDIAN_TRANSCRIPT_START: &str = ">>> TRANSCRIPT START\n"; @@ -152,12 +149,7 @@ pub(crate) async fn build_guardian_prompt_items_with_parent_turn( | GuardianApprovalRequest::McpToolCall { .. } | GuardianApprovalRequest::RequestPermissions { .. } => PlannedActionKind::Command, }, - reason: reasons.retry.or(reasons.approval).map(|reason| { - truncate_text( - &reason, - TruncationPolicy::Tokens(GUARDIAN_MAX_APPROVAL_REASON_TOKENS), - ) - }), + reason: reasons.retry.or(reasons.approval), }; let permissions = parent_context .map(|context| { diff --git a/codex-rs/guardian-context/Cargo.toml b/codex-rs/guardian-context/Cargo.toml index 1942443089..cc4f4cf734 100644 --- a/codex-rs/guardian-context/Cargo.toml +++ b/codex-rs/guardian-context/Cargo.toml @@ -16,6 +16,7 @@ workspace = true codex-context-fragments = { workspace = true } codex-history = { workspace = true } codex-protocol = { workspace = true } +codex-utils-output-truncation = { workspace = true } serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } diff --git a/codex-rs/guardian-context/src/composition.rs b/codex-rs/guardian-context/src/composition.rs index d67ee06798..1c421d3751 100644 --- a/codex-rs/guardian-context/src/composition.rs +++ b/codex-rs/guardian-context/src/composition.rs @@ -17,6 +17,8 @@ use codex_protocol::models::ImageReference; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::TruncationPolicy; use codex_protocol::user_input::UserInput; +use codex_utils_output_truncation::TruncationPolicy as OutputTruncationPolicy; +use codex_utils_output_truncation::truncate_text; use crate::ActionPresentation; use crate::BudgetPriority; @@ -31,6 +33,8 @@ use crate::TranscriptRecord; use crate::TruncationObservation; use crate::retained_instructions::RetainedInstructionKind; +const MAX_APPROVAL_REASON_TOKENS: usize = 512; + /// Consumer framing after the host has selected a full or delta transcript. pub enum ContextPresentation<'a> { SyncFull { session_id: &'a str }, @@ -148,7 +152,7 @@ impl CollectedContext { .unwrap_or_default() } - /// Frames the selected transcript while preserving structured text through admission. + /// Applies consumer caps and frames the selected transcript before request admission. pub fn compose( self, presentation: ContextPresentation<'_>, @@ -380,7 +384,15 @@ impl CollectedContext { SectionDelivery::user_content(items), ) } - ContextSection::PlannedAction(planned) => { + ContextSection::PlannedAction(mut planned) => { + if action != ActionPresentation::Async { + planned.reason = planned.reason.map(|reason| { + truncate_text( + &reason, + OutputTruncationPolicy::Tokens(MAX_APPROVAL_REASON_TOKENS), + ) + }); + } let mut items = planned .render(action) .into_iter() diff --git a/codex-rs/guardian-context/src/enforcement_tests.rs b/codex-rs/guardian-context/src/enforcement_tests.rs index ecd4b20d7b..0bdead4da7 100644 --- a/codex-rs/guardian-context/src/enforcement_tests.rs +++ b/codex-rs/guardian-context/src/enforcement_tests.rs @@ -261,50 +261,73 @@ fn recovery_shortens_older_history_only_after_optional_evidence() { #[test] fn planned_action_budget_omits_descriptions_without_changing_arguments() { + let reason = format!("reason-start {} reason-end", "x".repeat(/*n*/ 10_000)); let action = crate::PlannedAction { json: r#"{"tool":"write_record","arguments":{"description":"required payload"}}"# .to_owned(), kind: crate::PlannedActionKind::Command, - reason: None, + reason: Some(reason), tool_descriptions: Some("optional tool description ".repeat(/*n*/ 100)), }; - let required = action.render(crate::ActionPresentation::SyncFull); - let context = crate::CollectedContext { - sections: vec![crate::ContextSection::PlannedAction(action)], - } - .compose( - crate::ContextPresentation::SyncFull { - session_id: "review", - }, - crate::PreparedTranscript { - items: Vec::new(), - omission_note: None, - truncations: Vec::new(), - }, - ) - .unwrap(); - let mut required_items = required.iter().map(|item| text(item)).collect::>(); - required_items.push(text("evidence omitted")); - let budget = crate::estimate_input_tokens(&user_message(required_items.clone())) + 100; - let selected = context - .enforce_budget( - RequestBudget { - max_input_tokens: budget, - existing_context_tokens: 0, + for (presentation, action_presentation) in [ + ( + crate::ContextPresentation::SyncFull { + session_id: "review", + }, + crate::ActionPresentation::SyncFull, + ), + ( + crate::ContextPresentation::SyncDelta { + session_id: "review", + }, + crate::ActionPresentation::SyncDelta, + ), + ] { + let mut bounded = action.clone(); + bounded.reason = bounded.reason.map(|reason| { + codex_utils_output_truncation::truncate_text( + &reason, + codex_utils_output_truncation::TruncationPolicy::Tokens(/*tokens*/ 512), + ) + }); + let required = bounded.render(action_presentation); + let context = crate::CollectedContext { + sections: vec![crate::ContextSection::PlannedAction(action.clone())], + } + .compose( + presentation, + crate::PreparedTranscript { + items: Vec::new(), + omission_note: None, + truncations: Vec::new(), }, - "evidence omitted".to_owned(), - HistoryTruncation::Preserve, ) .unwrap(); - // The sync preamble is also required and remains ahead of the action. - let messages = selected.into_messages(); - let ResponseItem::Message { content, .. } = &messages[0] else { - panic!("expected user evidence"); - }; - assert_eq!( - &content[content.len() - required_items.len()..], - required_items - ); + let mut required_items = required.iter().map(|item| text(item)).collect::>(); + required_items.push(text("evidence omitted")); + let budget = crate::estimate_input_tokens(&user_message(required_items.clone())) + + section_tokens(&context.sections[0]) + + 100; + let selected = context + .enforce_budget( + RequestBudget { + max_input_tokens: budget, + existing_context_tokens: 0, + }, + "evidence omitted".to_owned(), + HistoryTruncation::Preserve, + ) + .unwrap(); + // The sync preamble is also required and remains ahead of the action. + let messages = selected.into_messages(); + let ResponseItem::Message { content, .. } = &messages[0] else { + panic!("expected user evidence"); + }; + assert_eq!( + &content[content.len() - required_items.len()..], + required_items + ); + } } #[test] diff --git a/codex-rs/guardian-context/src/lib.rs b/codex-rs/guardian-context/src/lib.rs index ad2e13f02a..91038ff753 100644 --- a/codex-rs/guardian-context/src/lib.rs +++ b/codex-rs/guardian-context/src/lib.rs @@ -162,7 +162,7 @@ pub struct SectionInput<'a> { pub root_conversation: &'a [GuardianRootMessage], /// Bounded, role-labeled answers selected from the host-owned context snapshot. pub trusted_user_answers: &'a [String], - /// Exact action JSON and reason, already bounded by the requesting host. + /// Exact action JSON and optional reason; sync composition bounds the reason. pub planned_action: Option<&'a PlannedAction>, /// Sync-only restrictions resolved from the parent execution environment. pub permissions: Option<&'a PermissionContext>,