github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-19 16:17:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Avoid duplicate sync reviews in Guardian classifier continuations (#49795)

Browse Source 
## Why

Retained Guardian classifier conversations already contain previously delivered synchronous reviews. Appending those reviews again repeats evidence and consumes input budget.

## What changed

Track each review completion with a stable identity in host-only history metadata. Filter already delivered reviews before composing and budgeting continuation input, while preserving distinct completions even when their text is identical.

Fresh conversations receive the review evidence again, and shortened messages no longer count as complete delivery. Delivery metadata stays out of model requests.

## Testing

Add coverage for delivering retained reviews once, appending newly completed reviews, restoring evidence after failed streams, and retaining reviews when history lacks complete delivery metadata.

GitOrigin-RevId: b8d58ffdd7d9c484676af707c65fa50020aae73c
This commit is contained in:
felixxia-oai
2026-10-01 01:13:29 +00:00
committed by copyberry
parent 726f1492db
commit 47a8bd7321
21 changed files with 300 additions and 70 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst
Generated
BIN
View File
Binary file not shown.
codex-rs/app-server/tests/suite/v2/guardian_stateful_async_tests.rs
+3
View File
@@ -3,6 +3,9 @@ use codex_protocol::openai_models::AsyncClassifierMode;
use test_case::test_case;
#[test_case(GuardianRisk::Low, ThreadLifecycle::New, ModelReviewRequirement::Optional; "low")]
#[test_case(GuardianRisk::Low, ThreadLifecycle::ReviewContinuations, ModelReviewRequirement::Optional; "retained_sync_review_delivered_once")]
#[test_case(GuardianRisk::High, ThreadLifecycle::ReviewContinuations, ModelReviewRequirement::Optional; "new_sync_review_appended")]
#[test_case(GuardianRisk::LowWithStreamFailure, ThreadLifecycle::ReviewContinuations, ModelReviewRequirement::Optional; "sync_review_restored_after_failed_stream")]
#[test_case(GuardianRisk::Low, ThreadLifecycle::RootUserRestriction, ModelReviewRequirement::Optional; "new_user_instruction_resets_conversation")]
#[test_case(GuardianRisk::LowWithStreamFailure, ThreadLifecycle::New, ModelReviewRequirement::Optional; "failure_after_early_score")]
#[test_case(GuardianRisk::Low, ThreadLifecycle::RootRestrictionDuringClassification, ModelReviewRequirement::Optional; "authorization_changed_during_classification")]
codex-rs/app-server/tests/suite/v2/guardian_v2.rs
+73 -8
View File
@@ -179,6 +179,7 @@ struct MockResponsesState {
guardian_requests: Mutex<Vec<Value>>,
luna_requests: Mutex<Vec<Value>>,
luna_connections: AtomicUsize,
luna_completions: AtomicUsize,
root_thread_id: Mutex<Option<String>>,
allow_luna: Notify,
allow_guardian_review: Notify,
@@ -242,6 +243,7 @@ enum GuardianToolScope {
#[derive(Clone, Copy)]
enum ThreadLifecycle {
New,
ReviewContinuations,
RequiredModelSwitch,
UserInputRestriction,
UserInputEmpty,
@@ -621,14 +623,17 @@ async fn luna_response(state: &MockResponsesState, request: Value) -> Vec<Value>
} else {
"high"
};
let completion = state.luna_completions.fetch_add(1, Ordering::Relaxed);
let response_id = format!("luna-score-{completion}");
let message_id = format!("luna-score-message-{completion}");
vec![
responses::ev_response_created("luna-score"),
responses::ev_response_created(&response_id),
responses::ev_output_text_delta(classification),
responses::ev_assistant_message("luna-score-message", classification),
responses::ev_assistant_message(&message_id, classification),
if state.fail_after_classification {
json!({"type":"response.failed", "response":{"id":"luna-score", "error":{"code":"server_error", "message":"stream failed after score"}}})
json!({"type":"response.failed", "response":{"id":response_id, "error":{"code":"server_error", "message":"stream failed after score"}}})
} else {
responses::ev_completed("luna-score")
responses::ev_completed(&response_id)
},
]
}
@@ -690,6 +695,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
classifier_mode: codex_protocol::openai_models::AsyncClassifierMode,
) -> Result<()> {
let fail_after_classification = matches!(risk, GuardianRisk::LowWithStreamFailure);
let review_continuations = matches!(lifecycle, ThreadLifecycle::ReviewContinuations);
let risk = if fail_after_classification {
GuardianRisk::Low
} else {
@@ -727,7 +733,8 @@ async fn guardian_v2_routes_scoped_tool_approvals(
GuardianRisk::Threshold => (0.5, 2),
GuardianRisk::High => (0.95, 2),
};
let expected_guardian_reviews = expected_guardian_reviews
let expected_guardian_reviews = (expected_guardian_reviews
+ usize::from(review_continuations && expected_guardian_reviews == 2))
* if matches!(review_outcome, ReviewOutcome::Malformed) {
3
} else {
@@ -741,6 +748,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
review_outcome,
transcript_content,
mcp_server_name: Some(server_name),
mcp_tool_sequence: review_continuations.then_some(&[TEST_TOOL_NAME; 3]),
root_worker: lifecycle.uses_root_worker(),
root_user_restriction: matches!(lifecycle, ThreadLifecycle::RootUserRestriction),
root_user_input_restriction: lifecycle.has_root_user_input(),
@@ -932,6 +940,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
}
let original_thread_id = match lifecycle {
ThreadLifecycle::New
| ThreadLifecycle::ReviewContinuations
| ThreadLifecycle::RequiredModelSwitch
| ThreadLifecycle::UserInputRestriction
| ThreadLifecycle::UserInputEmpty
@@ -980,6 +989,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
.await?;
let thread = match lifecycle {
ThreadLifecycle::New
| ThreadLifecycle::ReviewContinuations
| ThreadLifecycle::RequiredModelSwitch
| ThreadLifecycle::UserInputRestriction
| ThreadLifecycle::UserInputEmpty
@@ -1203,6 +1213,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
if matches!(
lifecycle,
ThreadLifecycle::New
| ThreadLifecycle::ReviewContinuations
| ThreadLifecycle::Resume
| ThreadLifecycle::Fork
| ThreadLifecycle::RootUserRestriction
@@ -1216,7 +1227,9 @@ async fn guardian_v2_routes_scoped_tool_approvals(
.as_array()
.expect("second Luna request input should be an array");
assert_eq!(
input.iter().any(|item| item["id"] == "luna-score-message"),
input.iter().any(|item| item["id"]
.as_str()
.is_some_and(|id| id.starts_with("luna-score-message-"))),
retained
);
if retained {
@@ -1455,6 +1468,53 @@ async fn guardian_v2_routes_scoped_tool_approvals(
wait_for_guardian_reviews(responses_state.as_ref(), expected_guardian_reviews).await?;
}
responses_state.allow_luna.notify_one();
if review_continuations {
let third_sample = wait_for_luna_request(responses_state.as_ref(), /*index*/ 2).await?;
assert!(
!third_sample.to_string().contains("guardian_review_ids"),
"review delivery metadata must stay out of the model request"
);
let previous_reviews = sync_review_fragments(&second_sample);
let reviews = sync_review_fragments(&third_sample);
let has_new_review = matches!(risk, GuardianRisk::High);
assert_eq!(previous_reviews.len(), 1);
assert_eq!(reviews.len(), 1 + usize::from(has_new_review));
assert_eq!(reviews[0], previous_reviews[0]);
if has_new_review {
assert!(reviews[1].contains("guardian-action-1"));
}
let input = third_sample["input"]
.as_array()
.expect("third classifier input");
if classifier_mode == "conversation" && !fail_after_classification {
let previous = second_sample["input"]
.as_array()
.expect("second classifier input");
assert_eq!(&input[..previous.len()], previous);
assert_eq!(
input
.iter()
.filter_map(|item| item["id"].as_str())
.filter(|id| id.starts_with("luna-score-message-"))
.collect::<Vec<_>>(),
vec!["luna-score-message-0", "luna-score-message-1"],
);
let delta = json!({ "input": &input[previous.len()..] });
assert_eq!(
sync_review_fragments(&delta),
reviews[1..],
"a retained continuation must append only newly completed sync reviews"
);
} else {
assert!(
!input.iter().any(|item| item["id"]
.as_str()
.is_some_and(|id| id.starts_with("luna-score-message-"))),
"snapshot and failed-stream recovery must start with fresh history"
);
}
responses_state.allow_luna.notify_one();
}
} else {
responses_state.allow_guardian_review.notify_one();
}
@@ -1567,7 +1627,10 @@ async fn guardian_v2_routes_scoped_tool_approvals(
.into_iter()
.filter(|method| method == "autoApprovalReview/strictReviewRequired")
.count();
assert_eq!(strict_review_count, usize::from(requires_strict_review));
assert_eq!(
strict_review_count,
usize::from(requires_strict_review) * if review_continuations { 2 } else { 1 }
);
if requires_strict_review {
let review_started: ItemGuardianApprovalReviewStartedNotification = timeout(
TIMEOUT,
@@ -1756,7 +1819,9 @@ async fn guardian_v2_routes_scoped_tool_approvals(
.as_array()
.expect("fresh classifier input");
assert!(
!input.iter().any(|item| item["id"] == "luna-score-message"),
!input.iter().any(|item| item["id"]
.as_str()
.is_some_and(|id| id.starts_with("luna-score-message-"))),
"new user instructions must invalidate the previous classifier conversation"
);
let text = serde_json::to_string(input)?;
codex-rs/core/src/context/guardian_review_evidence.rs
+3
View File
@@ -125,6 +125,7 @@ impl GuardianReviewEvidence {
return;
};
let review = Arc::new(GuardianReviewEvidenceRecord {
delivery_id: codex_protocol::ResponseItemId::new("guardian_review"),
completed_at_ms,
authorization_version,
review_context_revision,
@@ -170,6 +171,8 @@ impl GuardianReviewEvidence {
/// Structured synchronous-review evidence retained for Guardian V2 classification.
#[derive(Debug)]
pub struct GuardianReviewEvidenceRecord {
/// Identifies this immutable completion even if the same action is reviewed again.
pub delivery_id: codex_protocol::ResponseItemId,
pub authorization_version: GuardianAuthorizationVersion,
pub review_context_revision: u64,
pub root_authorization_version: Option<GuardianAuthorizationVersion>,
codex-rs/ext/guardian-v2/src/async_scorer/classification.rs
+6 -3
View File
@@ -197,14 +197,17 @@ impl Classification {
&& review.root_review_context_revision == root_review_context_revision
})
.map(|review| {
let review = render_review_evidence(ReviewEvidence {
let rendered = render_review_evidence(ReviewEvidence {
correlation: &review.correlation,
decision: &review.decision,
action: &review.action,
rationale: review.rationale.as_deref(),
});
truncations.extend(review.truncations);
GuardianReviewEvidenceFragment::new(review.body).render()
truncations.extend(rendered.truncations);
codex_guardian_context::PreviousReview {
id: review.delivery_id.clone(),
fragment: GuardianReviewEvidenceFragment::new(rendered.body).render(),
}
})
.collect::<Vec<_>>();
let transcript =
codex-rs/ext/guardian-v2/src/async_scorer/request.rs
+8 -9
View File
@@ -1,7 +1,7 @@
//! Admits classifier requests before moving committed history, without advancing the cursor.
//! The complete immutable prefix, setup and new evidence share the classifier input limit.
//! Instruction delivery metadata stays with history; previously delivered evidence is
//! omitted from new input before budgeting, using the same instruction filtering as sync.
//! Instruction and sync-review delivery metadata stay with history. Previously delivered
//! evidence is omitted before budgeting, using the same instruction filtering as sync.
use codex_guardian_context::TranscriptCursor;
use codex_guardian_context::TruncationObservation;
@@ -35,13 +35,12 @@ impl LunaSamplingRequest {
}) {
return None;
}
let (mut context, _) = evidence.clone().compose(cursor).ok()?;
context.retain_new_instructions(
conversation
.snapshot()
.map(|snapshot| snapshot.history().as_slice())
.unwrap_or_default(),
);
let history = conversation
.snapshot()
.map(|snapshot| snapshot.history().as_slice())
.unwrap_or_default();
let (mut context, _) = evidence.clone().compose(cursor, history).ok()?;
context.retain_new_instructions(history);
if let Some(snapshot) = conversation.snapshot() {
context.retain_images(|image, _| {
!snapshot.history().iter().any(|item| match &item.item {
codex-rs/ext/guardian-v2/src/async_scorer/request_tests.rs
+6 -5
View File
@@ -40,8 +40,11 @@ fn collect(history: &History, images: &[ContentItem]) -> CollectedTranscript {
reason: None,
};
let reviews =
PreviousReviews::try_from_fragments(vec!["Approved reading README.md only.".to_owned()])
.unwrap();
PreviousReviews::try_from_fragments(vec![codex_guardian_context::PreviousReview {
id: ResponseItemId::from_server("review-readme".to_owned()),
fragment: "Approved reading README.md only.".to_owned(),
}])
.unwrap();
TranscriptConfig::default()
.collect_context(ContextInput {
target: ContextTarget::Async,
@@ -153,9 +156,7 @@ fn preparation_preserves_committed_prefix_and_selects_only_new_entries_and_image
assert!(
matches!(&second.input[committed.len()].item, ResponseItem::Message { role, .. } if role == "user")
);
assert!(second.input[committed.len()..].iter().any(
|item| matches!(&item.item, ResponseItem::Message { role, .. } if role == "developer")
));
assert!(!delta.contains("Approved reading README.md only."));
// A rebuilt conversation must receive the current images again.
let rebuilt = config
.prepare_retained(&evidence, &mut ConversationState::default(), INPUT_BUDGET)
codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs
+12 -2
View File
@@ -698,7 +698,12 @@ async fn sampler_reuses_parent_compaction_only_for_matching_model_hashes() -> Re
request.parent_compaction_hash = parent_hash.map(str::to_owned);
request.input.insert(
/*index*/ 0,
PreviousReviews::try_from_fragments(vec!["trusted review".to_owned()])?.into_message(),
PreviousReviews::try_from_fragments(vec![codex_guardian_context::PreviousReview {
id: ResponseItemId::from_server("review-trusted".to_owned()),
fragment: "trusted review".to_owned(),
}])?
.into_annotated_message()
.into_item(),
);
let result = sampler.sample(request).await;
@@ -992,7 +997,12 @@ async fn sampler_retries_expired_websockets_on_another_warm_connection() -> Resu
let mut request = sample_request("turn-1");
request.input.insert(
/*index*/ 0,
PreviousReviews::try_from_fragments(vec!["trusted review".to_owned()])?.into_message(),
PreviousReviews::try_from_fragments(vec![codex_guardian_context::PreviousReview {
id: ResponseItemId::from_server("review-trusted".to_owned()),
fragment: "trusted review".to_owned(),
}])?
.into_annotated_message()
.into_item(),
);
request.input.insert(
/*index*/ 1,
codex-rs/ext/guardian-v2/src/async_scorer/transcript.rs
+4 -2
View File
@@ -88,7 +88,7 @@ impl TranscriptConfig {
&self,
input: ContextInput<'_>,
) -> Result<RenderedContext, SectionError> {
let (mut context, _) = self.collect_context(input)?.compose(/*cursor*/ None)?;
let (mut context, _) = self.collect_context(input)?.compose(/*cursor*/ None, &[])?;
context.deduplicate_transcript_instructions();
Ok(context)
}
@@ -178,9 +178,11 @@ impl CollectedTranscript {
}
pub(super) fn compose(
self,
mut self,
cursor: Option<TranscriptCursor>,
reviewer_history: &[codex_history::ResponseItemEnvelope],
) -> Result<(ComposedContext, TranscriptCursor), SectionError> {
self.context.retain_new_reviews(reviewer_history);
let (selection, next) = self.select(cursor);
let (entries, offset, presentation) = match selection {
TranscriptSelection::Full(entries) => (entries, 0, ContextPresentation::Async),
codex-rs/guardian-context/src/budget.rs
+2 -2
View File
@@ -152,7 +152,7 @@ impl ComposedContext {
.iter()
.map(|item| &item.content)
.fold(SectionCost::default(), SectionCost::add_content),
SectionDelivery::Message(message) => match message.content.as_ref() {
SectionDelivery::Message(message) => match &message.content.item {
ResponseItem::Message { content, .. } => content
.iter()
.fold(SectionCost::default(), SectionCost::add_content),
@@ -214,7 +214,7 @@ pub(super) fn section_tokens(section: &SectionOutput) -> usize {
.iter()
.map(|item| content_tokens(&item.content))
.fold(content_framing_tokens(content.len()), usize::saturating_add),
SectionDelivery::Message(message) => estimate_input_tokens(&message.content),
SectionDelivery::Message(message) => estimate_input_tokens(&message.content.item),
}
}
codex-rs/guardian-context/src/budget_tests.rs
+5 -4
View File
@@ -33,11 +33,12 @@ fn section_costs_keep_multimodal_payloads_separate() {
},
SectionOutput {
id: "trusted",
delivery: SectionDelivery::Message(crate::Budgeted::required(Box::new(message(
vec![ContentItem::InputText {
delivery: SectionDelivery::Message(crate::Budgeted::required(Box::new(
message(vec![ContentItem::InputText {
text: "verified".to_owned(),
}],
)))),
}])
.into(),
))),
},
],
truncations: Vec::new(),
codex-rs/guardian-context/src/composition.rs
+8 -6
View File
@@ -49,7 +49,7 @@ pub struct CollectedContext {
#[derive(Clone, PartialEq)]
pub(crate) enum SectionDelivery {
UserContent(Vec<Budgeted<ContentItem>>),
Message(Budgeted<Box<ResponseItem>>),
Message(Budgeted<Box<ResponseItemEnvelope>>),
}
/// Rendered evidence with a stable identity, independent of its source type.
@@ -137,20 +137,22 @@ impl CollectedContext {
ContextSection::PreviousReviews(reviews) => (
6,
"previous_reviews",
SectionDelivery::Message(Budgeted::required(Box::new(reviews.into_message()))),
SectionDelivery::Message(Budgeted::required(Box::new(
reviews.into_annotated_message(),
))),
),
ContextSection::TrustedTool(tool) => (
7,
"trusted_tool",
SectionDelivery::Message(Budgeted::required(Box::new(
ContextualUserFragment::into(tool),
ResponseItemEnvelope::new(ContextualUserFragment::into(tool)),
))),
),
ContextSection::TrustedSkills(skills) => (
8,
"trusted_skills",
SectionDelivery::Message(Budgeted::required(Box::new(
ContextualUserFragment::into(skills),
ResponseItemEnvelope::new(ContextualUserFragment::into(skills)),
))),
),
ContextSection::RootConversation { items } => {
@@ -210,7 +212,7 @@ impl CollectedContext {
SectionOutput {
id: "conversation_transcript",
delivery: SectionDelivery::Message(Budgeted {
content: message,
content: Box::new(ResponseItemEnvelope::new(*message)),
retention: entry.retention,
source: entry.source,
}),
@@ -425,7 +427,7 @@ impl ComposedContext {
std::mem::take(&mut metadata),
));
}
messages.push(ResponseItemEnvelope::new(*message.content));
messages.push(*message.content);
}
}
}
codex-rs/guardian-context/src/composition_tests.rs
+7 -4
View File
@@ -5,9 +5,12 @@ use pretty_assertions::assert_eq;
#[test]
fn delivery_preserves_arbitrary_message_boundaries_and_rejects_them_for_sync() {
let message = crate::PreviousReviews::try_from_fragments(vec!["host-attested review".into()])
.unwrap()
.into_message();
let message = crate::PreviousReviews::try_from_fragments(vec![crate::PreviousReview {
id: codex_protocol::ResponseItemId::new("review"),
fragment: "host-attested review".into(),
}])
.unwrap()
.into_annotated_message();
let sections = || {
vec![
SectionOutput {
@@ -41,7 +44,7 @@ fn delivery_preserves_arbitrary_message_boundaries_and_rejects_them_for_sync() {
text: "second".into()
}
]),
message.clone(),
message.item.clone(),
user_message(vec![ContentItem::InputText {
text: "third".into()
}]),
codex-rs/guardian-context/src/enforcement.rs
+1 -1
View File
@@ -271,7 +271,7 @@ impl ComposedContext {
{
self.truncations.push(TruncationObservation {
component: section.id,
original_bytes: serde_json::to_vec(&message.content)
original_bytes: serde_json::to_vec(&message.content.item)
.map_or(usize::MAX, |bytes| bytes.len()),
retained_bytes: 0,
});
codex-rs/guardian-context/src/enforcement_tests.rs
+8 -5
View File
@@ -143,9 +143,12 @@ fn planned_action_budget_omits_descriptions_without_changing_arguments() {
#[test]
fn budget_reserves_existing_context_and_preserves_required_messages() {
let trusted = crate::PreviousReviews::try_from_fragments(vec!["verified review".to_owned()])
.unwrap()
.into_message();
let trusted = crate::PreviousReviews::try_from_fragments(vec![crate::PreviousReview {
id: codex_protocol::ResponseItemId::new("review"),
fragment: "verified review".to_owned(),
}])
.unwrap()
.into_annotated_message();
let image = ContentItem::InputImage {
image: ImageReference::Inline {
image_url: "data:image/png;base64,AAAA".to_owned(),
@@ -213,7 +216,7 @@ fn budget_reserves_existing_context_and_preserves_required_messages() {
text("latest tool evidence"),
image.clone()
]),
trusted.clone(),
trusted.item.clone(),
user_message(vec![text("exact action"), text("evidence omitted")]),
]
);
@@ -231,7 +234,7 @@ fn budget_reserves_existing_context_and_preserves_required_messages() {
without_image.into_messages(),
vec![
user_message(vec![text("user restriction"), text("latest tool evidence")]),
trusted.clone(),
trusted.item.clone(),
user_message(vec![text("exact action"), text("evidence omitted")]),
]
);
codex-rs/guardian-context/src/lib.rs
+1
View File
@@ -95,6 +95,7 @@ pub use trusted_skills::TrustedSkills;
pub use trusted_tool::TrustedTool;
mod reviews;
pub use reviews::MAX_PREVIOUS_REVIEWS;
pub use reviews::PreviousReview;
pub use reviews::PreviousReviews;
pub use reviews::RenderedReviewEvidence;
pub use reviews::ReviewEvidence;
codex-rs/guardian-context/src/registry_tests.rs
+4 -3
View File
@@ -200,9 +200,10 @@ fn reused_registry_preserves_section_identity_and_source_roles() {
super::GuardianRootMessage::IncompleteRootInstructions,
];
let answers = ["assistant: Publish?\nuser: No.\n".to_string()];
let reviews = super::PreviousReviews::try_from_fragments(vec![
"<guardian_sync_review>debug-secret review</guardian_sync_review>".to_string(),
])
let reviews = super::PreviousReviews::try_from_fragments(vec![super::PreviousReview {
id: codex_protocol::ResponseItemId::new("review"),
fragment: "<guardian_sync_review>debug-secret review</guardian_sync_review>".to_string(),
}])
.unwrap();
let permissions = super::PermissionContext {
environment_id: None,
codex-rs/guardian-context/src/reviews.rs
+52 -6
View File
@@ -2,9 +2,11 @@
//! Construction validates count and rendered size without rewriting evidence.
//! The host selects authorization-valid records and attests each rendered body;
//! neither cached-decision validity nor review storage belongs to this module.
//! Delivery identities remain host-only and follow complete messages into committed history.
use serde_json::json;
use crate::CollectedContext;
use crate::ContextSection;
use crate::SectionContributor;
use crate::SectionError;
@@ -12,6 +14,9 @@ use crate::SectionInput;
use crate::SectionScope;
use crate::TruncationObservation;
use crate::truncate_text as truncate_entry;
use codex_history::CodexHarnessMetadata;
use codex_history::ResponseItemEnvelope;
use codex_protocol::ResponseItemId;
use codex_protocol::models::ContentItem;
use codex_protocol::models::ResponseItem;
use codex_protocol::protocol::TruncationPolicy;
@@ -32,7 +37,14 @@ pub struct ReviewEvidence<'a> {
/// Host-attested, already-bounded review fragments delivered only to async review.
#[derive(Clone, PartialEq)]
pub struct PreviousReviews {
fragments: Vec<String>,
fragments: Vec<PreviousReview>,
}
/// A bounded review fragment and its host-owned immutable completion identity.
#[derive(Clone, PartialEq)]
pub struct PreviousReview {
pub id: ResponseItemId,
pub fragment: String,
}
impl std::fmt::Debug for PreviousReviews {
@@ -48,10 +60,12 @@ impl PreviousReviews {
/// Validates the count and size of complete fragments, including their markers.
/// The host remains responsible for provenance and authorization validity.
/// Rejects oversized evidence without truncating or dropping any records.
pub fn try_from_fragments(fragments: Vec<String>) -> Result<Self, SectionError> {
pub fn try_from_fragments(fragments: Vec<PreviousReview>) -> Result<Self, SectionError> {
let max_bytes = TruncationPolicy::Tokens(MAX_REVIEW_FRAGMENT_TOKENS).byte_budget();
if fragments.len() > MAX_PREVIOUS_REVIEWS
|| fragments.iter().any(|fragment| fragment.len() > max_bytes)
|| fragments
.iter()
.any(|review| review.fragment.len() > max_bytes)
{
return Err(SectionError::EvidenceLimitExceeded {
section: "previous_reviews",
@@ -62,8 +76,13 @@ impl PreviousReviews {
/// Keeps the existing developer role and individual content-item boundaries.
/// Source actions and rationales remain evidence, never new authorization.
pub fn into_message(self) -> ResponseItem {
ResponseItem::Message {
pub fn into_annotated_message(self) -> ResponseItemEnvelope {
let ids = self
.fragments
.iter()
.map(|review| review.id.clone())
.collect();
let item = ResponseItem::Message {
id: None,
role: "developer".to_owned(),
content: std::iter::once(ContentItem::InputText {
@@ -75,15 +94,42 @@ impl PreviousReviews {
.chain(
self.fragments
.into_iter()
.map(|text| ContentItem::InputText { text }),
.map(|review| ContentItem::InputText {
text: review.fragment,
}),
)
.collect(),
phase: None,
internal_chat_message_metadata_passthrough: None,
};
ResponseItemEnvelope {
item,
metadata: Some(CodexHarnessMetadata {
guardian_review_ids: ids,
..Default::default()
}),
}
}
}
impl CollectedContext {
/// Appends only sync reviews not already represented in committed classifier history.
pub fn retain_new_reviews(&mut self, history: &[ResponseItemEnvelope]) {
self.sections.retain_mut(|section| {
let ContextSection::PreviousReviews(reviews) = section else {
return true;
};
reviews.fragments.retain(|review| {
!history
.iter()
.filter_map(|item| item.metadata.as_ref())
.any(|metadata| metadata.guardian_review_ids.contains(&review.id))
});
!reviews.fragments.is_empty()
});
}
}
pub(crate) struct PreviousReviewsSection;
impl SectionContributor for PreviousReviewsSection {
codex-rs/guardian-context/src/reviews_tests.rs
+84 -4
View File
@@ -7,16 +7,24 @@ use pretty_assertions::assert_eq;
fn previous_reviews_checks_count_and_size_without_rewriting_fragments() {
let max_bytes = TruncationPolicy::Tokens(MAX_REVIEW_FRAGMENT_TOKENS).byte_budget();
let fragment = "é".repeat(max_bytes / 2);
let fragments = vec![fragment.clone(); MAX_PREVIOUS_REVIEWS];
let fragments = vec![
PreviousReview {
id: ResponseItemId::new("review"),
fragment: fragment.clone(),
};
MAX_PREVIOUS_REVIEWS
];
let reviews = PreviousReviews::try_from_fragments(fragments.clone()).unwrap();
let ResponseItem::Message { content, .. } = reviews.into_message() else {
let ResponseItem::Message { content, .. } = reviews.into_annotated_message().item else {
panic!("expected a review message");
};
assert_eq!(
&content[1..],
fragments
.into_iter()
.map(|text| ContentItem::InputText { text })
.map(|review| ContentItem::InputText {
text: review.fragment
})
.collect::<Vec<_>>()
.as_slice()
);
@@ -25,10 +33,82 @@ fn previous_reviews_checks_count_and_size_without_rewriting_fragments() {
vec![format!("{fragment}a")],
] {
assert_eq!(
PreviousReviews::try_from_fragments(invalid),
PreviousReviews::try_from_fragments(
invalid
.into_iter()
.map(|fragment| PreviousReview {
id: ResponseItemId::new("review"),
fragment,
})
.collect()
),
Err(SectionError::EvidenceLimitExceeded {
section: "previous_reviews",
})
);
}
}
#[test]
fn review_delivery_requires_complete_host_metadata_and_preserves_distinct_completions() {
let first = PreviousReview {
id: ResponseItemId::new("review"),
fragment: "Approved reading README.md.".to_owned(),
};
// A different completion remains new evidence even when its text is identical.
let second = PreviousReview {
id: ResponseItemId::new("review"),
fragment: first.fragment.clone(),
};
let first_message = PreviousReviews::try_from_fragments(vec![first.clone()])
.unwrap()
.into_annotated_message();
let second_message = PreviousReviews::try_from_fragments(vec![second.clone()])
.unwrap()
.into_annotated_message();
let mut shortened = first_message.clone();
shortened
.metadata
.as_mut()
.unwrap()
.mark_retained_sources_incomplete();
let both = vec![first, second.clone()];
for (history, expected) in [
(Vec::new(), both.clone()),
(
vec![ResponseItemEnvelope::new(first_message.item.clone())],
both.clone(),
),
(vec![shortened], both.clone()),
(vec![first_message.clone()], vec![second]),
(vec![first_message, second_message], Vec::new()),
] {
let mut context = CollectedContext {
sections: vec![ContextSection::PreviousReviews(
PreviousReviews::try_from_fragments(both.clone()).unwrap(),
)],
};
context.retain_new_reviews(&history);
let delivered = context
.compose(
crate::ContextPresentation::Async,
crate::RenderedTranscript {
items: Vec::new(),
omission_note: None,
truncations: Vec::new(),
},
)
.unwrap()
.into_annotated_messages();
let expected = if expected.is_empty() {
Vec::new()
} else {
vec![
PreviousReviews::try_from_fragments(expected)
.unwrap()
.into_annotated_message(),
]
};
assert_eq!(delivered, expected);
}
}
codex-rs/guardian-context/tests/cache_prefix.rs
+7 -5
View File
@@ -73,10 +73,12 @@ fn changing_attestations_preserves_history_before_the_current_action() {
let profile = ContextProfile::asynchronous();
let mut previous_prefix = None;
for generation in 0..2 {
let reviews = PreviousReviews::try_from_fragments(vec![format!(
"Host-attested decision {generation}: denied."
)])
.unwrap();
let reviews =
PreviousReviews::try_from_fragments(vec![codex_guardian_context::PreviousReview {
id: codex_protocol::ResponseItemId::new("review"),
fragment: format!("Host-attested decision {generation}: denied."),
}])
.unwrap();
let tool = TrustedTool {
server: format!("server-{generation}"),
connector_id: None,
@@ -130,7 +132,7 @@ fn changing_attestations_preserves_history_before_the_current_action() {
assert_eq!(
suffix,
[
reviews.into_message(),
reviews.into_annotated_message().into_item(),
ContextualUserFragment::into(tool),
ContextualUserFragment::into(skills),
user_message(action.render(ActionPresentation::Async)),
codex-rs/history/src/lib.rs
+6 -1
View File
@@ -65,6 +65,10 @@ pub struct CodexHarnessMetadata {
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub guardian_sources: Vec<RetainedSource>,
/// Completed sync reviews delivered by this complete Guardian message. Host-only.
#[serde(default, skip_serializing_if = "Vec::is_empty")]
pub guardian_review_ids: Vec<codex_protocol::ResponseItemId>,
/// This complete message delivered the meaning of retained source-order labels.
#[serde(default, skip_serializing_if = "std::ops::Not::not")]
pub guardian_source_order_guidance: bool,
@@ -143,8 +147,9 @@ where
}
impl CodexHarnessMetadata {
/// Shortened messages no longer prove delivery of complete original instructions.
/// Shortened messages no longer prove complete instruction or review delivery.
pub fn mark_retained_sources_incomplete(&mut self) {
self.guardian_review_ids.clear();
self.guardian_source_order_guidance = false;
if let Some(source) = &mut self.retained_source {
source.complete = false;