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,