From 3b5483307d70da48ece3da8cefcaae9a19888651 Mon Sep 17 00:00:00 2001 From: felixxia-oai Date: Tue, 3 Nov 2026 11:54:30 +0000 Subject: [PATCH] Parse Guardian review responses once (#59136) ## What changed Introduce `GuardianReviewResponse` to retain both the parsed assessment and the original response text. Reuse the stored result for authorization checks, denial reconsideration, review record capture, and final assessment extraction instead of reparsing the response. Preserve raw text for review records and keep invalid responses as completed turns so history and session reuse remain unchanged. ## Testing Extend review record coverage to wrapped denials and invalid responses, and verify that invalid assessments remain retryable while missing assessments do not. GitOrigin-RevId: 50c19e788a8d57f951c6140d66306b3538515adc --- .../core/src/guardian/review_session_setup.rs | 4 +- .../core/src/guardian/review_session_tests.rs | 13 ++++-- .../ext/guardian-reviewer/src/execution.rs | 5 ++- .../ext/guardian-reviewer/src/feedback.rs | 9 ++--- .../guardian-reviewer/src/feedback_tests.rs | 17 +++++--- codex-rs/ext/guardian-reviewer/src/lib.rs | 3 +- codex-rs/ext/guardian-reviewer/src/outcome.rs | 40 +++++++++++++++++-- .../guardian-reviewer/src/reconsideration.rs | 12 +++--- .../ext/guardian-reviewer/src/retry_tests.rs | 11 ++++- 9 files changed, 85 insertions(+), 29 deletions(-) diff --git a/codex-rs/core/src/guardian/review_session_setup.rs b/codex-rs/core/src/guardian/review_session_setup.rs index 64955bacbe..f279a1b385 100644 --- a/codex-rs/core/src/guardian/review_session_setup.rs +++ b/codex-rs/core/src/guardian/review_session_setup.rs @@ -283,8 +283,8 @@ impl ReviewerRequest for PreparedReview { // All checkpoint capture and lock acquisition must precede validation. No await // may follow a valid allow before publication; new guidance during capture must // invalidate the attempt without overwriting the last committed review. - if let GuardianReviewSessionOutcome::Completed(Ok(Some(message))) = &result.outcome - && codex_guardian_reviewer::parse_guardian_assessment(message).is_ok_and(|assessment| { + if let GuardianReviewSessionOutcome::Completed(Ok(Some(response))) = &result.outcome + && response.assessment().is_ok_and(|assessment| { assessment.outcome == super::super::GuardianAssessmentOutcome::Allow }) && let Some(authorization) = &self.params.authorization diff --git a/codex-rs/core/src/guardian/review_session_tests.rs b/codex-rs/core/src/guardian/review_session_tests.rs index db4218b685..d5e1062e57 100644 --- a/codex-rs/core/src/guardian/review_session_tests.rs +++ b/codex-rs/core/src/guardian/review_session_tests.rs @@ -3,6 +3,7 @@ use crate::agents_md_manager::AgentsMdManager; use crate::context::ContextualUserFragment; use crate::context_manager::ContextManager; use crate::session::Submission; +use codex_guardian_reviewer::GuardianReviewResponse; use codex_guardian_reviewer::ReviewerRequest; use codex_guardian_reviewer::guardian_output_contract_prompt; use codex_history::CodexHarnessMetadata; @@ -735,7 +736,10 @@ async fn run_review_on_reused_session_waits_for_submitted_turn() { let GuardianReviewSessionOutcome::Completed(Ok(last_agent_message)) = outcome else { panic!("expected submitted turn completion"); }; - assert_eq!(last_agent_message.as_deref(), Some("fresh")); + assert_eq!( + last_agent_message.as_ref().map(GuardianReviewResponse::raw), + Some("fresh") + ); assert_eq!(analytics_result.time_to_first_token_ms, Some(42)); assert_eq!(disposition, SessionDisposition::Reusable); } @@ -836,7 +840,7 @@ async fn wait_for_guardian_review_ignores_prior_turn_errors() { let GuardianReviewSessionOutcome::Completed(Ok(last_agent_message)) = outcome else { panic!("expected current turn completion"); }; - assert_eq!(last_agent_message, None); + assert!(last_agent_message.is_none()); assert_eq!(analytics_result.time_to_first_token_ms, Some(42)); assert_eq!(disposition, SessionDisposition::Reusable); assert!(turn_completed); @@ -924,7 +928,10 @@ async fn wait_for_guardian_review_ignores_prior_turn_aborts() { let GuardianReviewSessionOutcome::Completed(Ok(last_agent_message)) = outcome else { panic!("expected current turn completion"); }; - assert_eq!(last_agent_message.as_deref(), Some("fresh")); + assert_eq!( + last_agent_message.as_ref().map(GuardianReviewResponse::raw), + Some("fresh") + ); assert_eq!(analytics_result.time_to_first_token_ms, Some(42)); assert_eq!(disposition, SessionDisposition::Reusable); assert!(turn_completed); diff --git a/codex-rs/ext/guardian-reviewer/src/execution.rs b/codex-rs/ext/guardian-reviewer/src/execution.rs index 5e1f553e46..4a01539791 100644 --- a/codex-rs/ext/guardian-reviewer/src/execution.rs +++ b/codex-rs/ext/guardian-reviewer/src/execution.rs @@ -16,6 +16,7 @@ use codex_protocol::turn_input::TurnInputSubmission; use tokio::time::Instant; use tokio_util::sync::CancellationToken; +use crate::GuardianReviewResponse; use crate::GuardianReviewSessionOutcome; use crate::SessionDisposition; @@ -133,7 +134,9 @@ pub async fn wait_for_guardian_review( }; } return ReviewTurnResult { - outcome: GuardianReviewSessionOutcome::Completed(Ok(turn_complete.last_agent_message)), + outcome: GuardianReviewSessionOutcome::Completed(Ok( + turn_complete.last_agent_message.map(GuardianReviewResponse::new), + )), disposition: SessionDisposition::Reusable, turn_completed: true, }; diff --git a/codex-rs/ext/guardian-reviewer/src/feedback.rs b/codex-rs/ext/guardian-reviewer/src/feedback.rs index 5e5b34a411..2741e8a332 100644 --- a/codex-rs/ext/guardian-reviewer/src/feedback.rs +++ b/codex-rs/ext/guardian-reviewer/src/feedback.rs @@ -2,7 +2,6 @@ //! The host supplies captured context; records outlive short-lived reviewer sessions. use crate::GuardianReviewSessionOutcome; -use crate::parse_guardian_assessment; use codex_feedback::GuardianReviewRecord; use codex_protocol::ThreadId; use codex_protocol::models::ResponseItem; @@ -62,13 +61,13 @@ impl<'a> FailedReviewFeedback<'a> { return None; } let (status, decision) = match outcome { - GuardianReviewSessionOutcome::Completed(Ok(Some(decision))) => { - match parse_guardian_assessment(decision) { + GuardianReviewSessionOutcome::Completed(Ok(Some(response))) => { + match response.assessment() { Ok(assessment) if assessment.outcome == GuardianAssessmentOutcome::Allow => { return None; } - Ok(_) => ("denied", Some(decision.as_str())), - Err(_) => ("invalid_decision", Some(decision.as_str())), + Ok(_) => ("denied", Some(response.raw())), + Err(_) => ("invalid_decision", Some(response.raw())), } } GuardianReviewSessionOutcome::Completed(Ok(None)) => ("missing_decision", None), diff --git a/codex-rs/ext/guardian-reviewer/src/feedback_tests.rs b/codex-rs/ext/guardian-reviewer/src/feedback_tests.rs index c9874ff5c1..b05f5fcb07 100644 --- a/codex-rs/ext/guardian-reviewer/src/feedback_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/feedback_tests.rs @@ -1,14 +1,21 @@ use super::*; +use crate::GuardianReviewResponse; use pretty_assertions::assert_eq; use serde_json::json; -#[test] -fn capture_honors_privacy_and_keeps_oversized_context_action_and_decision() -> anyhow::Result<()> { +#[test_case::test_case(r#"{"outcome":"deny","rationale":"Missing approval."}"#, "denied"; "denial")] +#[test_case::test_case(r#"preface {"outcome":"deny"}"#, "denied"; "wrapped denial")] +#[test_case::test_case("not valid guardian json", "invalid_decision"; "invalid response")] +fn capture_honors_privacy_and_keeps_oversized_context_action_and_decision( + decision: &str, + status: &str, +) -> anyhow::Result<()> { let thread_id = ThreadId::new(); let reviewer_thread_id = ThreadId::new(); - let decision = r#"{"outcome":"deny","rationale":"Missing approval."}"#; let action = r#"{"command":"git push"}"#; - let outcome = GuardianReviewSessionOutcome::Completed(Ok(Some(decision.to_owned()))); + let outcome = GuardianReviewSessionOutcome::Completed(Ok(Some(GuardianReviewResponse::new( + decision.to_owned(), + )))); for settings in [ ReviewFeedbackSettings { enabled: false, @@ -49,7 +56,7 @@ fn capture_honors_privacy_and_keeps_oversized_context_action_and_decision() -> a "target_item_id": "push-call", "reviewer_thread_id": reviewer_thread_id, "model": "review-model", - "status": "denied", + "status": status, "decision": decision, "action": action, "action_truncated": false, diff --git a/codex-rs/ext/guardian-reviewer/src/lib.rs b/codex-rs/ext/guardian-reviewer/src/lib.rs index 1abc03bde2..3b28f13291 100644 --- a/codex-rs/ext/guardian-reviewer/src/lib.rs +++ b/codex-rs/ext/guardian-reviewer/src/lib.rs @@ -68,7 +68,7 @@ pub use approval_request_json::guardian_approval_request_to_json; pub use assessment::GuardianAssessment; pub use assessment::guardian_output_contract_prompt; pub use assessment::guardian_output_schema; -pub use assessment::parse_guardian_assessment; +pub(crate) use assessment::parse_guardian_assessment; pub(crate) use circuit_breaker::AUTO_REVIEW_DENIAL_WINDOW_SIZE; pub(crate) use circuit_breaker::GuardianRejectionCircuitBreaker; pub(crate) use circuit_breaker::GuardianRejectionCircuitBreakerAction; @@ -78,6 +78,7 @@ pub use model::ReviewModel; pub use model::select_review_model; pub use outcome::GuardianReviewError; pub use outcome::GuardianReviewOutcome; +pub use outcome::GuardianReviewResponse; pub use outcome::GuardianReviewSessionOutcome; pub use planned_action::build_guardian_planned_action; pub use retry::GuardianReviewSessionLimits; diff --git a/codex-rs/ext/guardian-reviewer/src/outcome.rs b/codex-rs/ext/guardian-reviewer/src/outcome.rs index 3f6b378af9..612dcaeae4 100644 --- a/codex-rs/ext/guardian-reviewer/src/outcome.rs +++ b/codex-rs/ext/guardian-reviewer/src/outcome.rs @@ -1,4 +1,5 @@ -//! Distinguishes completed assessments from failures without assigning risk to errors. +//! Retains parsed reviewer responses through feedback capture and distinguishes +//! completed assessments from failures without assigning risk to errors. use crate::GuardianAssessment; use codex_analytics::GuardianReviewFailureReason; @@ -79,7 +80,7 @@ impl GuardianReviewError { #[derive(Debug)] pub enum GuardianReviewSessionOutcome { - Completed(anyhow::Result>), + Completed(anyhow::Result>), PromptBuildFailed(anyhow::Error), InputBudgetExceeded, SessionFailed { @@ -92,12 +93,43 @@ pub enum GuardianReviewSessionOutcome { StaleAuthorization, } +/// A completed response parsed once, with its original text retained for feedback. +/// Invalid JSON remains a completed response so history and session reuse are unchanged. +pub struct GuardianReviewResponse { + raw: String, + assessment: anyhow::Result, +} + +impl GuardianReviewResponse { + pub fn new(raw: String) -> Self { + let assessment = crate::parse_guardian_assessment(&raw); + Self { raw, assessment } + } + + pub fn raw(&self) -> &str { + &self.raw + } + + pub fn assessment(&self) -> Result<&GuardianAssessment, &anyhow::Error> { + self.assessment.as_ref() + } +} + +impl std::fmt::Debug for GuardianReviewResponse { + fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter + .debug_struct("GuardianReviewResponse") + .field("valid", &self.assessment.is_ok()) + .finish_non_exhaustive() + } +} + impl GuardianReviewSessionOutcome { /// Extract the assessment produced by one reviewer session, preserving failures. pub fn into_assessment(self) -> Result { match self { - Self::Completed(Ok(Some(message))) => { - crate::parse_guardian_assessment(&message).map_err(GuardianReviewError::parse) + Self::Completed(Ok(Some(response))) => { + response.assessment.map_err(GuardianReviewError::parse) } Self::Completed(Ok(None)) => Err(GuardianReviewError::session(anyhow::anyhow!( "guardian review completed without an assessment payload" diff --git a/codex-rs/ext/guardian-reviewer/src/reconsideration.rs b/codex-rs/ext/guardian-reviewer/src/reconsideration.rs index 1fe9c39673..9c9eb9cc86 100644 --- a/codex-rs/ext/guardian-reviewer/src/reconsideration.rs +++ b/codex-rs/ext/guardian-reviewer/src/reconsideration.rs @@ -7,7 +7,6 @@ use crate::ReviewDeadline; use crate::ReviewTurnResult; use crate::ReviewerRuntime; use crate::SessionDisposition; -use crate::parse_guardian_assessment; use crate::start_review_turn; use crate::wait_for_guardian_review; use codex_analytics::GuardianReviewAnalyticsResult; @@ -28,10 +27,11 @@ pub async fn reconsider_denial( analytics: &mut GuardianReviewAnalyticsResult, telemetry: &SessionTelemetry, ) -> ReviewTurnResult { - let GuardianReviewSessionOutcome::Completed(Ok(Some(message))) = &first.outcome else { + let GuardianReviewSessionOutcome::Completed(Ok(Some(response))) = &first.outcome else { return first; }; - if !parse_guardian_assessment(message) + if !response + .assessment() .is_ok_and(|assessment| assessment.outcome == GuardianAssessmentOutcome::Deny) { return first; @@ -53,9 +53,7 @@ pub async fn reconsider_denial( // aggregate token usage from the same session after both passes finish. analytics.time_to_first_token_ms = first_token_ms; let assessment = match &second.outcome { - GuardianReviewSessionOutcome::Completed(Ok(Some(message))) => { - parse_guardian_assessment(message).ok() - } + GuardianReviewSessionOutcome::Completed(Ok(Some(response))) => response.assessment().ok(), GuardianReviewSessionOutcome::Completed(Ok(None) | Err(_)) | GuardianReviewSessionOutcome::PromptBuildFailed(_) | GuardianReviewSessionOutcome::InputBudgetExceeded @@ -64,7 +62,7 @@ pub async fn reconsider_denial( | GuardianReviewSessionOutcome::TimedOut | GuardianReviewSessionOutcome::Aborted => None, }; - let outcome = match assessment.as_ref().map(|assessment| assessment.outcome) { + let outcome = match assessment.map(|assessment| assessment.outcome) { Some(GuardianAssessmentOutcome::Allow) => "allow", Some(GuardianAssessmentOutcome::Deny) => "deny", None => match &second.outcome { diff --git a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs index d5992e7941..6c75d25de2 100644 --- a/codex-rs/ext/guardian-reviewer/src/retry_tests.rs +++ b/codex-rs/ext/guardian-reviewer/src/retry_tests.rs @@ -1,6 +1,8 @@ #[cfg(test)] use super::*; use crate::GuardianAssessment; +use crate::GuardianReviewResponse; +use crate::GuardianReviewSessionOutcome; use codex_analytics::GuardianReviewFailureReason; use codex_protocol::protocol::GuardianAssessmentOutcome; use codex_protocol::protocol::GuardianRiskLevel; @@ -113,9 +115,16 @@ fn guardian_review_retry_only_retries_recoverable_errors() { false, ), ( - GuardianReviewOutcome::Error(GuardianReviewError::parse(anyhow::anyhow!("parse"))), + GuardianReviewSessionOutcome::Completed(Ok(Some(GuardianReviewResponse::new( + "invalid assessment".to_owned(), + )))) + .into(), true, ), + ( + GuardianReviewSessionOutcome::Completed(Ok(None)).into(), + false, + ), ( GuardianReviewOutcome::Error(GuardianReviewError::Timeout), false,