From c6596ea2d706b07f943ce4e7a26454555618b866 Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Fri, 17 Apr 2026 12:31:49 -0700 Subject: [PATCH] [codex-analytics] guardian review truncation --- .../core/src/guardian/approval_request.rs | 64 +++++++----- codex-rs/core/src/guardian/mod.rs | 2 +- codex-rs/core/src/guardian/prompt.rs | 20 ++-- codex-rs/core/src/guardian/review.rs | 1 + codex-rs/core/src/guardian/review_session.rs | 28 +++--- codex-rs/core/src/guardian/tests.rs | 97 +++++++++++++++++-- 6 files changed, 159 insertions(+), 53 deletions(-) diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index 6d1d3f76af..72ec5fcccd 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -167,32 +167,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 { @@ -382,10 +399,13 @@ pub(crate) fn guardian_request_turn_id<'a>( } } -pub(crate) fn format_guardian_action_pretty( +pub(crate) fn format_guardian_action_pretty_with_truncation( 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/mod.rs b/codex-rs/core/src/guardian/mod.rs index d9647b6bf0..ccbdadcfe5 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -62,7 +62,7 @@ pub(crate) struct GuardianRejection { } #[cfg(test)] -use approval_request::format_guardian_action_pretty; +use approval_request::format_guardian_action_pretty_with_truncation; #[cfg(test)] use approval_request::guardian_assessment_action; #[cfg(test)] diff --git a/codex-rs/core/src/guardian/prompt.rs b/codex-rs/core/src/guardian/prompt.rs index 687b3e13a7..bc10f6d57d 100644 --- a/codex-rs/core/src/guardian/prompt.rs +++ b/codex-rs/core/src/guardian/prompt.rs @@ -19,7 +19,7 @@ use super::GUARDIAN_RECENT_ENTRY_LIMIT; use super::GuardianApprovalRequest; use super::GuardianAssessment; use super::TRUNCATION_TAG; -use super::approval_request::format_guardian_action_pretty; +use super::approval_request::format_guardian_action_pretty_with_truncation; /// Transcript entry retained for guardian review after filtering. #[derive(Debug, PartialEq, Eq)] @@ -56,6 +56,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. @@ -91,7 +92,7 @@ pub(crate) async fn build_guardian_prompt_items( parent_history_version: history.history_version(), transcript_entry_count: transcript_entries.len(), }; - let planned_action_json = format_guardian_action_pretty(&request)?; + let planned_action = format_guardian_action_pretty_with_truncation(&request)?; let prompt_shape = match mode { GuardianPromptMode::Full => GuardianPromptShape::Full, @@ -176,11 +177,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.text)); push_text(">>> APPROVAL REQUEST END\n".to_string()); Ok(GuardianPromptItems { items, transcript_cursor, + reviewed_action_truncated: planned_action.truncated, }) } @@ -240,7 +242,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, @@ -420,20 +422,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()); @@ -441,7 +443,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.rs b/codex-rs/core/src/guardian/review.rs index e0eda7db77..107c653e3d 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -280,6 +280,7 @@ impl GuardianReviewAnalyticsResult { result.guardian_model = Some(metadata.guardian_model); result.guardian_reasoning_effort = metadata.guardian_reasoning_effort; result.had_prior_review_context = Some(metadata.had_prior_review_context); + result.reviewed_action_truncated = metadata.reviewed_action_truncated; result.token_usage = metadata.token_usage; } diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index c1ea324d1b..5ae2d361a5 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -72,6 +72,7 @@ pub(crate) struct GuardianReviewSessionMetadata { pub(crate) guardian_model: String, pub(crate) guardian_reasoning_effort: Option, pub(crate) had_prior_review_context: bool, + pub(crate) reviewed_action_truncated: bool, pub(crate) token_usage: Option, } @@ -620,6 +621,7 @@ async fn run_review_on_session( guardian_model: params.model.clone(), guardian_reasoning_effort: params.reasoning_effort.map(|effort| effort.to_string()), had_prior_review_context: had_prior_review_context(&prompt_mode), + reviewed_action_truncated: false, token_usage: None, }; if send_followup_reminder { @@ -646,6 +648,7 @@ async fn run_review_on_session( prompt_mode, ) .await?; + let reviewed_action_truncated = prompt_items.reviewed_action_truncated; // A fresh Guardian review session may not have token info yet. Treat that // as a zero baseline so the first review can still report usage. let token_usage_at_review_start = review_session @@ -673,8 +676,9 @@ async fn run_review_on_session( }) .await?; - Ok::<(GuardianTranscriptCursor, TokenUsage), anyhow::Error>(( + Ok::<(GuardianTranscriptCursor, bool, TokenUsage), anyhow::Error>(( prompt_items.transcript_cursor, + reviewed_action_truncated, token_usage_at_review_start, )) }), @@ -684,16 +688,18 @@ async fn run_review_on_session( Ok(submit_result) => submit_result, Err(outcome) => return (outcome, false, guardian_metadata), }; - let (transcript_cursor, token_usage_at_review_start) = match submit_result { - Ok(submit_result) => submit_result, - Err(err) => { - return ( - GuardianReviewSessionOutcome::PromptBuildFailed(err), - false, - guardian_metadata, - ); - } - }; + let (transcript_cursor, reviewed_action_truncated, token_usage_at_review_start) = + match submit_result { + Ok(submit_result) => submit_result, + Err(err) => { + return ( + GuardianReviewSessionOutcome::PromptBuildFailed(err), + false, + guardian_metadata, + ); + } + }; + guardian_metadata.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 37aec2c3bc..3e1ad23646 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -570,11 +570,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:: patch: patch.clone(), }; - let rendered = format_guardian_action_pretty(&action)?; + let rendered = format_guardian_action_pretty_with_truncation(&action)?; - assert!(rendered.contains("\"tool\": \"apply_patch\"")); - assert!(rendered.contains(" anyhow: let GuardianReviewSessionResult { outcome: GuardianReviewOutcome::Completed(first_assessment), - .. + metadata: first_metadata, } = first_outcome else { panic!("expected first guardian assessment"); }; + let first_metadata = first_metadata.expect("first guardian session metadata"); let GuardianReviewSessionResult { outcome: GuardianReviewOutcome::Completed(second_assessment), - .. + metadata: second_metadata, } = second_outcome else { panic!("expected second guardian assessment"); }; + let second_metadata = second_metadata.expect("second guardian session metadata"); let GuardianReviewSessionResult { outcome: GuardianReviewOutcome::Completed(third_assessment), - .. + metadata: third_metadata, } = third_outcome else { panic!("expected third guardian assessment"); }; + let third_metadata = third_metadata.expect("third guardian session metadata"); 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, + codex_analytics::GuardianReviewSessionKind::TrunkNew + )); + assert!(matches!( + second_metadata.guardian_session_kind, + codex_analytics::GuardianReviewSessionKind::TrunkReused + )); + assert!(matches!( + third_metadata.guardian_session_kind, + codex_analytics::GuardianReviewSessionKind::TrunkReused + )); + ThreadId::from_string(&first_metadata.guardian_thread_id) + .expect("first guardian thread id should be a valid UUID"); + ThreadId::from_string(&second_metadata.guardian_thread_id) + .expect("second guardian thread id should be a valid UUID"); + ThreadId::from_string(&third_metadata.guardian_thread_id) + .expect("third guardian thread id should be a valid UUID"); + assert!(!first_metadata.had_prior_review_context); + assert!(second_metadata.had_prior_review_context); + assert!(third_metadata.had_prior_review_context); + 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);