From 3da7de59b6411caa96b56856ac87370518ac1653 Mon Sep 17 00:00:00 2001 From: swastik-oai Date: Wed, 28 Oct 2026 20:51:16 +0000 Subject: [PATCH] Add isolated Guardian assessments for prepared actions (#57834) ## What changed Add `ReviewInput` to bundle an approval request with its captured parent history and review context. Its `assess_isolated` method runs a fresh reviewer without publishing a live approval, honors deadlines and cancellation, and returns a `GuardianAssessment` or review error. Share review parameter construction with the live review path and extract assessment parsing and error conversion into `GuardianReviewSessionOutcome::into_assessment`. ## Testing Add a mock-backed test that verifies the isolated assessment is returned and the reviewer receives the captured history without instructions added after capture. GitOrigin-RevId: 10dd5fddf5fc58bfab124f5962a6b7f45d435c83 --- codex-rs/core/src/guardian/review.rs | 117 ++++++++++++++---- codex-rs/core/src/guardian/review_session.rs | 1 + .../core/src/guardian/review_session_setup.rs | 4 +- codex-rs/core/src/guardian/tests.rs | 54 ++++++++ codex-rs/ext/guardian-reviewer/src/outcome.rs | 55 ++++---- 5 files changed, 178 insertions(+), 53 deletions(-) diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index caa3810a7c..b2307e9728 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -9,6 +9,7 @@ use codex_analytics::GuardianApprovalRequestSource; use codex_analytics::GuardianReviewAnalyticsResult; use codex_core_plugins::PluginCommandAttribution; use codex_features::Feature; +use codex_guardian_reviewer::GuardianAssessment; use codex_guardian_reviewer::GuardianReviewError; use codex_guardian_reviewer::GuardianReviewOutcome; #[cfg(test)] @@ -29,6 +30,7 @@ use tokio_util::sync::CancellationToken; use crate::context::GuardianNodeReplPolicy; use crate::context::GuardianReviewEvidence; +use crate::context_manager::ContextManager; use crate::session::session::Session; use super::ApprovalRequestReasons; @@ -157,6 +159,87 @@ struct ReviewAttemptOptions { deadline: ReviewDeadline, } +/// A prepared action and the parent transcript to use as evidence for it. +pub(super) struct ReviewInput { + pub(super) session: Arc, + pub(super) context: GuardianReviewContext, + pub(super) history: ContextManager, + pub(super) request: GuardianApprovalRequest, + pub(super) reasons: ApprovalRequestReasons, +} + +impl ReviewInput { + fn into_params( + self, + config: GuardianReviewSessionConfig, + category: GuardianScope, + options: ReviewAttemptOptions, + ) -> GuardianReviewSessionParams { + GuardianReviewSessionParams { + parent_session: self.session, + parent_history: self.history, + reasoning_summary: self.context.reasoning_summary, + personality: self.context.personality, + parent_context: self.context, + spawn_config: config.spawn_config, + node_repl_policy: config.node_repl_policy, + request: self.request, + category, + reasons: self.reasons, + authorization: options.authorization, + schema: guardian_output_schema(), + review_model: config.review_model, + external_cancel: options.external_cancel, + deadline: options.deadline, + } + } + + /// Assess the supplied request in a fresh reviewer without publishing a live approval. + #[cfg_attr( + not(test), + expect(dead_code, reason = "consumed by the recorded-call stack layer") + )] + pub(super) async fn assess_isolated( + self, + deadline: ReviewDeadline, + cancellation: &CancellationToken, + ) -> Result { + let review = async { + let pool = self.session.guardian_review_session().ok_or_else(|| { + GuardianReviewError::session(anyhow::anyhow!("Guardian reviewer is unavailable")) + })?; + let config = guardian_review_session_config(&self.session, &self.context) + .await + .map_err(GuardianReviewError::prompt_build)?; + let category = self.request.guardian_scope(); + let params = self.into_params( + config, + category, + ReviewAttemptOptions { + authorization: None, + external_cancel: Some(cancellation.clone()), + deadline, + }, + ); + let prepared = super::review_session::prepare_review(params) + .await + .map_err(GuardianReviewError::prompt_build)?; + Ok::<_, GuardianReviewError>(pool.review_isolated(&prepared).await.0) + }; + let outcome = match run_before_review_deadline( + deadline, + Some(cancellation), + Box::pin(review), + ) + .await + { + Ok(result) => result?, + Err(interruption) => interruption, + }; + outcome.into_assessment() + } +} + /// Runs the guardian in a locked-down reusable review session. /// /// The guardian itself should not mutate state or trigger further approvals, so @@ -208,28 +291,18 @@ async fn run_guardian_review_session_before_deadline( ); } }; - let (session_outcome, session_analytics_result) = - Box::pin(super::review_session::run_guardian_review_session( - pool, - GuardianReviewSessionParams { - parent_session: Arc::clone(&session), - parent_context: context.clone(), - parent_history: session.clone_history().await, - spawn_config: session_config.spawn_config, - node_repl_policy: session_config.node_repl_policy, - request, - category, - reasons, - authorization: options.authorization, - schema: guardian_output_schema(), - review_model: session_config.review_model, - reasoning_summary: context.reasoning_summary, - personality: context.personality, - external_cancel: options.external_cancel, - deadline: options.deadline, - }, - )) - .await; + let input = ReviewInput { + history: session.clone_history().await, + session, + context, + request, + reasons, + }; + let params = input.into_params(session_config, category, options); + let (session_outcome, session_analytics_result) = Box::pin( + super::review_session::run_guardian_review_session(pool, params), + ) + .await; (session_outcome.into(), session_analytics_result) } diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 9a39edd428..65005cc725 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -5,6 +5,7 @@ #[path = "review_session_setup.rs"] mod setup; pub use setup::PreparedGuardianContext; +pub(super) use setup::prepare_review; pub use setup::prepare_review_prewarm; pub(crate) use setup::run_guardian_review_session; diff --git a/codex-rs/core/src/guardian/review_session_setup.rs b/codex-rs/core/src/guardian/review_session_setup.rs index 013e6ea3d6..64955bacbe 100644 --- a/codex-rs/core/src/guardian/review_session_setup.rs +++ b/codex-rs/core/src/guardian/review_session_setup.rs @@ -203,7 +203,7 @@ impl PreparedGuardianContext { } #[derive(Clone)] -pub(super) struct PreparedReview { +pub(in crate::guardian) struct PreparedReview { context: Arc, params: Arc, recovery_requested: Arc, @@ -360,7 +360,7 @@ pub(crate) async fn run_guardian_review_session( (outcome, analytics) } -pub(super) async fn prepare_review( +pub(in crate::guardian) async fn prepare_review( params: GuardianReviewSessionParams, ) -> anyhow::Result { let context = PreparedGuardianContext::prepare( diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 27af1196dc..b09c5431aa 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -205,6 +205,60 @@ fn guardian_exec_command_request(id: &str) -> GuardianApprovalRequest { } } +#[tokio::test] +async fn isolated_review_returns_assessment_for_prepared_action() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let assessment = serde_json::json!({ + "risk_level": "medium", + "user_authorization": "high", + "outcome": "allow", + "rationale": "Publishing the requested documentation change." + }); + let mock = mount_sse_once( + &server, + sse(vec![ + ev_assistant_message("assessment", &assessment.to_string()), + ev_completed("review"), + ]), + ) + .await; + let (session, turn) = guardian_test_session_and_turn(&server).await; + seed_guardian_parent_history(&session, &turn).await; + let history = session.clone_history().await; + let later = codex_protocol::models::ResponseInputItem::Message { + role: "user".into(), + content: vec![codex_protocol::models::input_text( + "Instruction added after capture.", + )], + phase: None, + }; + session + .record_conversation_items(&turn, turn.model_info(), &[later.into()]) + .await; + let input = review::ReviewInput { + history, + context: GuardianReviewContext::from(&turn), + request: guardian_exec_command_request("supplied-action"), + reasons: ApprovalRequestReasons::default(), + session, + }; + let result = input + .assess_isolated( + codex_guardian_reviewer::ReviewDeadline::Unlimited, + &CancellationToken::new(), + ) + .await + .expect("isolated assessment"); + assert_eq!(result, serde_json::from_value(assessment)?); + let request = mock.single_request(); + let rendered = serde_json::to_string(&request.input())?; + assert!(rendered.contains("Please check the repo visibility")); + assert!(rendered.contains("Need to push the reviewed docs fix.")); + assert!(!rendered.contains("Instruction added after capture.")); + Ok(()) +} + fn guardian_mcp_request(server: &str, tool_name: &str) -> GuardianApprovalRequest { GuardianApprovalRequest::McpToolCall { id: "mcp-1".to_string(), diff --git a/codex-rs/ext/guardian-reviewer/src/outcome.rs b/codex-rs/ext/guardian-reviewer/src/outcome.rs index 738ae555f2..3f6b378af9 100644 --- a/codex-rs/ext/guardian-reviewer/src/outcome.rs +++ b/codex-rs/ext/guardian-reviewer/src/outcome.rs @@ -92,43 +92,40 @@ pub enum GuardianReviewSessionOutcome { StaleAuthorization, } -impl From for GuardianReviewOutcome { - fn from(outcome: GuardianReviewSessionOutcome) -> Self { - match outcome { - GuardianReviewSessionOutcome::Completed(Ok(Some(message))) => { - match crate::parse_guardian_assessment(&message) { - Ok(assessment) => Self::Completed(assessment), - Err(error) => Self::Error(GuardianReviewError::parse(error)), - } +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) } - GuardianReviewSessionOutcome::Completed(Ok(None)) => { - Self::Error(GuardianReviewError::session(anyhow::anyhow!( - "guardian review completed without an assessment payload" - ))) - } - GuardianReviewSessionOutcome::Completed(Err(error)) => { - Self::Error(GuardianReviewError::session(error)) - } - GuardianReviewSessionOutcome::PromptBuildFailed(error) => { - Self::Error(GuardianReviewError::prompt_build(error)) - } - GuardianReviewSessionOutcome::InputBudgetExceeded => { - Self::Error(GuardianReviewError::InputBudgetExceeded) - } - GuardianReviewSessionOutcome::SessionFailed { + Self::Completed(Ok(None)) => Err(GuardianReviewError::session(anyhow::anyhow!( + "guardian review completed without an assessment payload" + ))), + Self::Completed(Err(error)) => Err(GuardianReviewError::session(error)), + Self::PromptBuildFailed(error) => Err(GuardianReviewError::prompt_build(error)), + Self::InputBudgetExceeded => Err(GuardianReviewError::InputBudgetExceeded), + Self::SessionFailed { error, error_info, retry_at, - } => Self::Error(GuardianReviewError::Session { + } => Err(GuardianReviewError::Session { message: error.to_string(), error_info, retry_at, }), - GuardianReviewSessionOutcome::TimedOut => Self::Error(GuardianReviewError::Timeout), - GuardianReviewSessionOutcome::Aborted => Self::Error(GuardianReviewError::Cancelled), - GuardianReviewSessionOutcome::StaleAuthorization => { - Self::Error(GuardianReviewError::StaleAuthorization) - } + Self::TimedOut => Err(GuardianReviewError::Timeout), + Self::Aborted => Err(GuardianReviewError::Cancelled), + Self::StaleAuthorization => Err(GuardianReviewError::StaleAuthorization), + } + } +} + +impl From for GuardianReviewOutcome { + fn from(outcome: GuardianReviewSessionOutcome) -> Self { + match outcome.into_assessment() { + Ok(assessment) => Self::Completed(assessment), + Err(error) => Self::Error(error), } } }