mirror of
https://github.com/openai/codex.git
synced 2026-11-02 19:43:24 +00:00
Improve Guardian report diagnostics (#42033)
## Why Reports from a large thread tree can exceed the rollout attachment limit, making the threads involved in a failed Guardian review harder to diagnose. ## What changed - Prioritize descendants with retained failed reviews when selecting bounded report rollouts, while always preserving the reported thread. - Include each selected thread's available Guardian trunk rollout alongside its conversation rollout. - Add a thread index attachment with selected filenames, bounded omission details, retained failure threads, and the process-wide discarded-record count. - Link failed-review records to their reviewed turn and target item when available. ## Testing - Cover failed-review prioritization, bounded omission metadata, discarded-record accounting, and turn/item linkage in Guardian report records. GitOrigin-RevId: ae7916c27a3c279e899cd5a6ead8a4b7e0cc26a1
This commit is contained in:
@@ -13,17 +13,29 @@ const MAX_BYTES: usize = 8 * 1024 * 1024;
|
||||
static RECORDS: Mutex<ReviewRecords> = Mutex::new(ReviewRecords {
|
||||
records: VecDeque::new(),
|
||||
bytes: 0,
|
||||
discarded_records: 0,
|
||||
});
|
||||
|
||||
/// A consistent snapshot of retained failures and their owning threads.
|
||||
pub struct GuardianReviewFailures {
|
||||
pub attachment: Option<FeedbackAttachment>,
|
||||
/// Unique threads, ordered by most recently retained failure first.
|
||||
pub thread_ids: Vec<ThreadId>,
|
||||
/// Process-wide evictions or oversized records, not a count for this task tree.
|
||||
pub process_discarded_records: usize,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct ReviewRecords {
|
||||
records: VecDeque<(ThreadId, Vec<u8>)>,
|
||||
bytes: usize,
|
||||
discarded_records: usize,
|
||||
}
|
||||
|
||||
impl ReviewRecords {
|
||||
fn push(&mut self, thread_id: ThreadId, record: Vec<u8>) {
|
||||
if record.len() + 1 > MAX_BYTES {
|
||||
self.discarded_records = self.discarded_records.saturating_add(1);
|
||||
return;
|
||||
}
|
||||
if self
|
||||
@@ -36,17 +48,19 @@ impl ReviewRecords {
|
||||
&& let Some((_, removed)) = self.records.remove(index)
|
||||
{
|
||||
self.bytes -= removed.len() + 1;
|
||||
self.discarded_records = self.discarded_records.saturating_add(1);
|
||||
}
|
||||
while self.records.len() >= MAX_RECORDS || self.bytes + record.len() + 1 > MAX_BYTES {
|
||||
if let Some((_, removed)) = self.records.pop_front() {
|
||||
self.bytes -= removed.len() + 1;
|
||||
self.discarded_records = self.discarded_records.saturating_add(1);
|
||||
}
|
||||
}
|
||||
self.bytes += record.len() + 1;
|
||||
self.records.push_back((thread_id, record));
|
||||
}
|
||||
|
||||
fn attachment(&self, thread_ids: &[ThreadId]) -> Option<FeedbackAttachment> {
|
||||
fn snapshot(&self, thread_ids: &[ThreadId]) -> GuardianReviewFailures {
|
||||
let thread_ids = thread_ids.iter().copied().collect::<HashSet<_>>();
|
||||
let mut buffer = Vec::new();
|
||||
for (thread_id, record) in &self.records {
|
||||
@@ -55,11 +69,24 @@ impl ReviewRecords {
|
||||
buffer.push(b'\n');
|
||||
}
|
||||
}
|
||||
(!buffer.is_empty()).then_some(FeedbackAttachment {
|
||||
let attachment = (!buffer.is_empty()).then_some(FeedbackAttachment {
|
||||
filename: "auto-review-failures.jsonl".to_string(),
|
||||
buffer,
|
||||
content_type: Some("application/x-ndjson".to_string()),
|
||||
})
|
||||
});
|
||||
let mut seen = HashSet::new();
|
||||
let thread_ids = self
|
||||
.records
|
||||
.iter()
|
||||
.rev()
|
||||
.filter(|(id, _)| thread_ids.contains(id) && seen.insert(*id))
|
||||
.map(|(id, _)| *id)
|
||||
.collect();
|
||||
GuardianReviewFailures {
|
||||
attachment,
|
||||
thread_ids,
|
||||
process_discarded_records: self.discarded_records,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -72,11 +99,11 @@ pub fn record_guardian_review_failure(thread_id: ThreadId, record: Vec<u8>) {
|
||||
}
|
||||
|
||||
/// Snapshot failures for the reported task tree without including other tasks' records.
|
||||
pub fn guardian_review_failures_attachment(thread_ids: &[ThreadId]) -> Option<FeedbackAttachment> {
|
||||
pub fn guardian_review_failures(thread_ids: &[ThreadId]) -> GuardianReviewFailures {
|
||||
RECORDS
|
||||
.lock()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner)
|
||||
.attachment(thread_ids)
|
||||
.snapshot(thread_ids)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -12,13 +12,18 @@ fn records_keep_recent_failures_and_select_the_requested_tree() {
|
||||
for index in 0..12 {
|
||||
records.push(root, format!(r#"{{"root":{index}}}"#).into_bytes());
|
||||
}
|
||||
let attachment = records
|
||||
.attachment(&[root, child])
|
||||
.expect("task-tree records");
|
||||
let snapshot = records.snapshot(&[root, child]);
|
||||
let attachment = snapshot.attachment.expect("task-tree records");
|
||||
let expected = std::iter::once("{\"child\":\"denied\"}\n".to_string())
|
||||
.chain((4..12).map(|index| format!("{{\"root\":{index}}}\n")))
|
||||
.collect::<String>();
|
||||
assert_eq!(attachment.buffer, expected.into_bytes());
|
||||
assert_eq!(snapshot.thread_ids, vec![root, child]);
|
||||
assert_eq!(snapshot.process_discarded_records, 4);
|
||||
let unrelated_snapshot = records.snapshot(&[unrelated]);
|
||||
assert_eq!(unrelated_snapshot.thread_ids, vec![unrelated]);
|
||||
// The diagnostic counter must not be mistaken for tree-specific omissions.
|
||||
assert_eq!(unrelated_snapshot.process_discarded_records, 4);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -37,10 +42,12 @@ fn record_count_and_bytes_are_bounded_across_threads() {
|
||||
assert_eq!(records.records.len(), 2);
|
||||
assert_eq!(
|
||||
records
|
||||
.attachment(&[thread_id])
|
||||
.snapshot(&[thread_id])
|
||||
.attachment
|
||||
.expect("bounded records")
|
||||
.buffer
|
||||
.len(),
|
||||
2 * (payload.len() + 1)
|
||||
);
|
||||
assert_eq!(records.discarded_records, MAX_RECORDS + 3);
|
||||
}
|
||||
|
||||
@@ -38,7 +38,8 @@ mod upload;
|
||||
pub use feedback_diagnostics::FEEDBACK_DIAGNOSTICS_ATTACHMENT_FILENAME;
|
||||
pub use feedback_diagnostics::FeedbackDiagnostic;
|
||||
pub use feedback_diagnostics::FeedbackDiagnostics;
|
||||
pub use guardian::guardian_review_failures_attachment;
|
||||
pub use guardian::GuardianReviewFailures;
|
||||
pub use guardian::guardian_review_failures;
|
||||
pub use guardian::record_guardian_review_failure;
|
||||
pub use report_upload::FeedbackDelivery;
|
||||
pub use report_upload::FeedbackTransport;
|
||||
|
||||
Reference in New Issue
Block a user