mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[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
This commit is contained in:
committed by
GitHub
Unverified
parent
4e7399c6b9
commit
37aadeaa13
@@ -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::<Vec<_>>(),
|
||||
),
|
||||
.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));
|
||||
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<Value> {
|
||||
@@ -482,8 +499,11 @@ pub(crate) fn guardian_request_turn_id<'a>(
|
||||
|
||||
pub(crate) fn format_guardian_action_pretty(
|
||||
action: &GuardianApprovalRequest,
|
||||
) -> serde_json::Result<String> {
|
||||
let mut value = guardian_approval_request_to_json(action)?;
|
||||
value = truncate_guardian_action_value(value);
|
||||
serde_json::to_string_pretty(&value)
|
||||
) -> serde_json::Result<FormattedGuardianAction> {
|
||||
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,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -59,6 +59,7 @@ impl GuardianTranscriptEntryKind {
|
||||
pub(crate) struct GuardianPromptItems {
|
||||
pub(crate) items: Vec<UserInput>,
|
||||
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(
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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("<truncated omitted_approx_tokens=\""));
|
||||
assert!(truncated.ends_with("suffix"));
|
||||
assert!(was_truncated);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -593,9 +594,27 @@ fn format_guardian_action_pretty_truncates_large_string_fields() -> serde_json::
|
||||
|
||||
let rendered = format_guardian_action_pretty(&action)?;
|
||||
|
||||
assert!(rendered.contains("\"tool\": \"apply_patch\""));
|
||||
assert!(rendered.contains("<truncated omitted_approx_tokens="));
|
||||
assert!(rendered.len() < patch.len());
|
||||
assert!(rendered.text.contains("\"tool\": \"apply_patch\""));
|
||||
assert!(rendered.text.contains("<truncated omitted_approx_tokens="));
|
||||
assert!(rendered.text.len() < patch.len());
|
||||
assert!(rendered.truncated);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[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(),
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -996,11 +1015,20 @@ async fn guardian_review_request_layout_matches_model_visible_request_snapshot()
|
||||
/*external_cancel*/ None,
|
||||
)
|
||||
.await;
|
||||
let (GuardianReviewOutcome::Completed(assessment), _) = outcome else {
|
||||
let (GuardianReviewOutcome::Completed(assessment), metadata) = outcome else {
|
||||
panic!("expected guardian assessment");
|
||||
};
|
||||
let guardian_thread_id = metadata
|
||||
.guardian_thread_id
|
||||
.as_deref()
|
||||
.expect("guardian thread id");
|
||||
assert_eq!(assessment.outcome, GuardianAssessmentOutcome::Allow);
|
||||
|
||||
assert_ne!(guardian_thread_id, session.conversation_id.to_string());
|
||||
ThreadId::from_string(guardian_thread_id).expect("guardian thread id should be a valid UUID");
|
||||
assert!(matches!(
|
||||
metadata.guardian_session_kind,
|
||||
Some(codex_analytics::GuardianReviewSessionKind::TrunkNew)
|
||||
));
|
||||
let request = request_log.single_request();
|
||||
let request_body = request.body_json();
|
||||
assert_eq!(
|
||||
@@ -1032,6 +1060,21 @@ async fn guardian_review_request_layout_matches_model_visible_request_snapshot()
|
||||
"required": ["outcome"]
|
||||
}))
|
||||
);
|
||||
let request_model = request_body
|
||||
.get("model")
|
||||
.and_then(|value| value.as_str())
|
||||
.expect("guardian request should include a model");
|
||||
let request_reasoning_effort = request_body
|
||||
.get("reasoning")
|
||||
.and_then(|reasoning| reasoning.get("effort"))
|
||||
.and_then(|value| value.as_str());
|
||||
assert_eq!(metadata.guardian_model.as_deref(), Some(request_model));
|
||||
assert_eq!(
|
||||
metadata.guardian_reasoning_effort.as_deref(),
|
||||
request_reasoning_effort
|
||||
);
|
||||
assert_eq!(metadata.had_prior_review_context, Some(false));
|
||||
|
||||
let mut settings = Settings::clone_current();
|
||||
settings.set_snapshot_path("snapshots");
|
||||
settings.set_prepend_module_to_snapshot(false);
|
||||
@@ -1235,18 +1278,63 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> 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);
|
||||
|
||||
Reference in New Issue
Block a user