From 37aadeaa13fc9b8b2f9cd93f56786e236bf9f8d1 Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Wed, 22 Apr 2026 01:35:29 -0700 Subject: [PATCH] [codex-analytics] guardian review truncation (#17695) ## Why The Guardian review event needs to report whether the action shown to Guardian was truncated. That field should come from the same truncation path used to build the Guardian prompt, rather than being inferred after the fact. ## What changed Plumbs truncation metadata through Guardian action formatting, prompt construction, review session execution, and analytics emission. `guardian_truncate_text` now reports both the rendered text and whether it inserted the truncation marker, and `reviewed_action_truncated` is set from that prompt-building result. This keeps the analytics field aligned with the model-visible reviewed action while preserving the existing Guardian prompt behavior. ## Verification - Guardian truncation tests cover both truncated and non-truncated action payloads. - Guardian review tests assert the review session metadata and truncation field are propagated. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/17695). * #17696 * __->__ #17695 * #17693 * #18278 * #18953 --- .../core/src/guardian/approval_request.rs | 62 ++++++---- codex-rs/core/src/guardian/prompt.rs | 16 +-- codex-rs/core/src/guardian/review_session.rs | 2 + codex-rs/core/src/guardian/tests.rs | 106 ++++++++++++++++-- 4 files changed, 149 insertions(+), 37 deletions(-) diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 471e05442..989c6ee11 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -184,32 +184,49 @@ fn guardian_command_source_tool_name(source: GuardianCommandSource) -> &'static } } -fn truncate_guardian_action_value(value: Value) -> Value { +fn truncate_guardian_action_value(value: Value) -> (Value, bool) { match value { - Value::String(text) => Value::String(guardian_truncate_text( - &text, - GUARDIAN_MAX_ACTION_STRING_TOKENS, - )), - Value::Array(values) => Value::Array( - values + 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(truncate_guardian_action_value) - .collect::>(), - ), + .map(|value| { + let (value, value_truncated) = truncate_guardian_action_value(value); + truncated |= value_truncated; + value + }) + .collect::>(); + (Value::Array(values), truncated) + } Value::Object(values) => { let mut entries = values.into_iter().collect::>(); entries.sort_by(|(left, _), (right, _)| left.cmp(right)); - Value::Object( - entries - .into_iter() - .map(|(key, value)| (key, truncate_guardian_action_value(value))) - .collect(), - ) + 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, + 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 { @@ -482,8 +499,11 @@ pub(crate) fn guardian_request_turn_id<'a>( pub(crate) fn format_guardian_action_pretty( action: &GuardianApprovalRequest, -) -> serde_json::Result { - let mut value = guardian_approval_request_to_json(action)?; - value = truncate_guardian_action_value(value); - serde_json::to_string_pretty(&value) +) -> serde_json::Result { + let value = guardian_approval_request_to_json(action)?; + let (value, truncated) = truncate_guardian_action_value(value); + Ok(FormattedGuardianAction { + text: serde_json::to_string_pretty(&value)?, + truncated, + }) } diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index 1a716f25b..adf9dd8d7 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -59,6 +59,7 @@ impl GuardianTranscriptEntryKind { pub(crate) struct GuardianPromptItems { pub(crate) items: Vec, pub(crate) transcript_cursor: GuardianTranscriptCursor, + pub(crate) reviewed_action_truncated: bool, } /// Points to the end of the transcript that the guardian has already reviewed. @@ -179,11 +180,12 @@ pub(crate) async fn build_guardian_prompt_items( .to_string(), ); push_text("Planned action JSON:\n".to_string()); - push_text(format!("{planned_action_json}\n")); + push_text(format!("{}\n", planned_action_json.text)); push_text(">>> APPROVAL REQUEST END\n".to_string()); Ok(GuardianPromptItems { items, transcript_cursor, + reviewed_action_truncated: planned_action_json.truncated, }) } @@ -243,7 +245,7 @@ fn render_guardian_transcript_entries_with_offset( } else { GUARDIAN_MAX_MESSAGE_ENTRY_TOKENS }; - let text = guardian_truncate_text(&entry.text, token_cap); + let (text, _) = guardian_truncate_text(&entry.text, token_cap); let rendered = format!( "[{}] {}: {}", index + entry_number_offset + 1, @@ -423,20 +425,20 @@ pub(crate) fn collect_guardian_transcript_entries( entries } -pub(crate) fn guardian_truncate_text(content: &str, token_cap: usize) -> String { +pub(crate) fn guardian_truncate_text(content: &str, token_cap: usize) -> (String, bool) { if content.is_empty() { - return String::new(); + return (String::new(), false); } let max_bytes = approx_bytes_for_tokens(token_cap); if content.len() <= max_bytes { - return content.to_string(); + return (content.to_string(), false); } let omitted_tokens = approx_tokens_from_byte_count(content.len().saturating_sub(max_bytes)); let marker = format!("<{TRUNCATION_TAG} omitted_approx_tokens=\"{omitted_tokens}\" />"); if max_bytes <= marker.len() { - return marker; + return (marker, true); } let available_bytes = max_bytes.saturating_sub(marker.len()); @@ -444,7 +446,7 @@ pub(crate) fn guardian_truncate_text(content: &str, token_cap: usize) -> String let suffix_budget = available_bytes.saturating_sub(prefix_budget); let (prefix, suffix) = split_guardian_truncation_bounds(content, prefix_budget, suffix_budget); - format!("{prefix}{marker}{suffix}") + (format!("{prefix}{marker}{suffix}"), true) } fn split_guardian_truncation_bounds( diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index e68024dbf..7e2fa59b4 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -672,6 +672,7 @@ async fn run_review_on_session( ); } }; + let reviewed_action_truncated = prompt_items.reviewed_action_truncated; let transcript_cursor = prompt_items.transcript_cursor; let token_usage_at_review_start = review_session .codex @@ -711,6 +712,7 @@ async fn run_review_on_session( } Err(outcome) => return (outcome, false, analytics_result), } + analytics_result.reviewed_action_truncated = reviewed_action_truncated; let outcome = wait_for_guardian_review(review_session, deadline, params.external_cancel.as_ref()).await; diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 9ce265e91..587d73af7 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -574,11 +574,12 @@ fn collect_guardian_transcript_entries_includes_recent_tool_calls_and_output() { fn guardian_truncate_text_keeps_prefix_suffix_and_xml_marker() { let content = "prefix ".repeat(200) + &" suffix".repeat(200); - let truncated = guardian_truncate_text(&content, /*token_cap*/ 20); + let (truncated, was_truncated) = guardian_truncate_text(&content, /*token_cap*/ 20); assert!(truncated.starts_with("prefix")); assert!(truncated.contains(" serde_json:: let rendered = format_guardian_action_pretty(&action)?; - assert!(rendered.contains("\"tool\": \"apply_patch\"")); - assert!(rendered.contains(" anyhow: ) .await; - let (GuardianReviewOutcome::Completed(first_assessment), _) = first_outcome else { + let (GuardianReviewOutcome::Completed(first_assessment), first_metadata) = first_outcome else { panic!("expected first guardian assessment"); }; - let (GuardianReviewOutcome::Completed(second_assessment), _) = second_outcome else { + let (GuardianReviewOutcome::Completed(second_assessment), second_metadata) = second_outcome + else { panic!("expected second guardian assessment"); }; - let (GuardianReviewOutcome::Completed(third_assessment), _) = third_outcome else { + let (GuardianReviewOutcome::Completed(third_assessment), third_metadata) = third_outcome else { panic!("expected third guardian assessment"); }; assert_eq!(first_assessment.outcome, GuardianAssessmentOutcome::Allow); assert_eq!(second_assessment.outcome, GuardianAssessmentOutcome::Allow); assert_eq!(third_assessment.outcome, GuardianAssessmentOutcome::Allow); + assert!(matches!( + first_metadata.guardian_session_kind, + Some(codex_analytics::GuardianReviewSessionKind::TrunkNew) + )); + assert!(matches!( + second_metadata.guardian_session_kind, + Some(codex_analytics::GuardianReviewSessionKind::TrunkReused) + )); + assert!(matches!( + third_metadata.guardian_session_kind, + Some(codex_analytics::GuardianReviewSessionKind::TrunkReused) + )); + ThreadId::from_string( + first_metadata + .guardian_thread_id + .as_deref() + .expect("first guardian thread id"), + ) + .expect("first guardian thread id should be a valid UUID"); + ThreadId::from_string( + second_metadata + .guardian_thread_id + .as_deref() + .expect("second guardian thread id"), + ) + .expect("second guardian thread id should be a valid UUID"); + ThreadId::from_string( + third_metadata + .guardian_thread_id + .as_deref() + .expect("third guardian thread id"), + ) + .expect("third guardian thread id should be a valid UUID"); + assert_eq!(first_metadata.had_prior_review_context, Some(false)); + assert_eq!(second_metadata.had_prior_review_context, Some(true)); + assert_eq!(third_metadata.had_prior_review_context, Some(true)); + assert_eq!( + first_metadata.guardian_thread_id, + second_metadata.guardian_thread_id + ); + assert_eq!( + second_metadata.guardian_thread_id, + third_metadata.guardian_thread_id + ); let requests = request_log.requests(); assert_eq!(requests.len(), 3);