diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index 6de2c0c546..23cdddd252 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server/tests/suite/v2/guardian_stateful_async_tests.rs b/codex-rs/app-server/tests/suite/v2/guardian_stateful_async_tests.rs index d430225bdd..e7d83f13e0 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_stateful_async_tests.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_stateful_async_tests.rs @@ -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")] diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index a8935aac26..1f7c841b0a 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -179,6 +179,7 @@ struct MockResponsesState { guardian_requests: Mutex>, luna_requests: Mutex>, luna_connections: AtomicUsize, + luna_completions: AtomicUsize, root_thread_id: Mutex>, 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 } 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!["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)?; diff --git a/codex-rs/core/src/context/guardian_review_evidence.rs b/codex-rs/core/src/context/guardian_review_evidence.rs index a8d996c911..426755efed 100644 --- a/codex-rs/core/src/context/guardian_review_evidence.rs +++ b/codex-rs/core/src/context/guardian_review_evidence.rs @@ -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, diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/classification.rs b/codex-rs/ext/guardian-v2/src/async_scorer/classification.rs index 23118ea7db..8e33cef6b9 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/classification.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/classification.rs @@ -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::>(); let transcript = diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/request.rs b/codex-rs/ext/guardian-v2/src/async_scorer/request.rs index 3e5f27c6fa..60602629ec 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/request.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/request.rs @@ -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 { diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/request_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/request_tests.rs index bd30e22e4e..9938bd22c3 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/request_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/request_tests.rs @@ -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) diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs index aa9eb7e9b8..4fee4370bd 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/sampler_tests.rs @@ -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, diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/transcript.rs b/codex-rs/ext/guardian-v2/src/async_scorer/transcript.rs index 23a25653f7..f30a282549 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/transcript.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/transcript.rs @@ -88,7 +88,7 @@ impl TranscriptConfig { &self, input: ContextInput<'_>, ) -> Result { - 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, + 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), diff --git a/codex-rs/guardian-context/src/budget.rs b/codex-rs/guardian-context/src/budget.rs index 80fc189e18..56cc8a09d1 100644 --- a/codex-rs/guardian-context/src/budget.rs +++ b/codex-rs/guardian-context/src/budget.rs @@ -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), } } diff --git a/codex-rs/guardian-context/src/budget_tests.rs b/codex-rs/guardian-context/src/budget_tests.rs index fb6cbf6f79..a5ffada0d4 100644 --- a/codex-rs/guardian-context/src/budget_tests.rs +++ b/codex-rs/guardian-context/src/budget_tests.rs @@ -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(), diff --git a/codex-rs/guardian-context/src/composition.rs b/codex-rs/guardian-context/src/composition.rs index 8cdc7658a7..1aaf221bf8 100644 --- a/codex-rs/guardian-context/src/composition.rs +++ b/codex-rs/guardian-context/src/composition.rs @@ -49,7 +49,7 @@ pub struct CollectedContext { #[derive(Clone, PartialEq)] pub(crate) enum SectionDelivery { UserContent(Vec>), - Message(Budgeted>), + Message(Budgeted>), } /// 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); } } } diff --git a/codex-rs/guardian-context/src/composition_tests.rs b/codex-rs/guardian-context/src/composition_tests.rs index eda0a4ccd2..8c53b1d571 100644 --- a/codex-rs/guardian-context/src/composition_tests.rs +++ b/codex-rs/guardian-context/src/composition_tests.rs @@ -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() }]), diff --git a/codex-rs/guardian-context/src/enforcement.rs b/codex-rs/guardian-context/src/enforcement.rs index c44a5da7a4..c844fee647 100644 --- a/codex-rs/guardian-context/src/enforcement.rs +++ b/codex-rs/guardian-context/src/enforcement.rs @@ -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, }); diff --git a/codex-rs/guardian-context/src/enforcement_tests.rs b/codex-rs/guardian-context/src/enforcement_tests.rs index 68df87ebf7..05cb2f325b 100644 --- a/codex-rs/guardian-context/src/enforcement_tests.rs +++ b/codex-rs/guardian-context/src/enforcement_tests.rs @@ -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")]), ] ); diff --git a/codex-rs/guardian-context/src/lib.rs b/codex-rs/guardian-context/src/lib.rs index e6220cdd9d..8670313177 100644 --- a/codex-rs/guardian-context/src/lib.rs +++ b/codex-rs/guardian-context/src/lib.rs @@ -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; diff --git a/codex-rs/guardian-context/src/registry_tests.rs b/codex-rs/guardian-context/src/registry_tests.rs index 32e68818dc..9fd02669ea 100644 --- a/codex-rs/guardian-context/src/registry_tests.rs +++ b/codex-rs/guardian-context/src/registry_tests.rs @@ -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![ - "debug-secret review".to_string(), - ]) + let reviews = super::PreviousReviews::try_from_fragments(vec![super::PreviousReview { + id: codex_protocol::ResponseItemId::new("review"), + fragment: "debug-secret review".to_string(), + }]) .unwrap(); let permissions = super::PermissionContext { environment_id: None, diff --git a/codex-rs/guardian-context/src/reviews.rs b/codex-rs/guardian-context/src/reviews.rs index 5e15cb6589..7e85d6eed5 100644 --- a/codex-rs/guardian-context/src/reviews.rs +++ b/codex-rs/guardian-context/src/reviews.rs @@ -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, + fragments: Vec, +} + +/// 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) -> Result { + pub fn try_from_fragments(fragments: Vec) -> Result { 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 { diff --git a/codex-rs/guardian-context/src/reviews_tests.rs b/codex-rs/guardian-context/src/reviews_tests.rs index 18aa07a145..87dd422bb1 100644 --- a/codex-rs/guardian-context/src/reviews_tests.rs +++ b/codex-rs/guardian-context/src/reviews_tests.rs @@ -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::>() .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); + } +} diff --git a/codex-rs/guardian-context/tests/cache_prefix.rs b/codex-rs/guardian-context/tests/cache_prefix.rs index 89ad061f84..a1db94dbaa 100644 --- a/codex-rs/guardian-context/tests/cache_prefix.rs +++ b/codex-rs/guardian-context/tests/cache_prefix.rs @@ -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)), diff --git a/codex-rs/history/src/lib.rs b/codex-rs/history/src/lib.rs index b117bfa558..ab23c00401 100644 --- a/codex-rs/history/src/lib.rs +++ b/codex-rs/history/src/lib.rs @@ -65,6 +65,10 @@ pub struct CodexHarnessMetadata { #[serde(default, skip_serializing_if = "Vec::is_empty")] pub guardian_sources: Vec, + /// Completed sync reviews delivered by this complete Guardian message. Host-only. + #[serde(default, skip_serializing_if = "Vec::is_empty")] + pub guardian_review_ids: Vec, + /// 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;