mirror of
https://github.com/openai/codex.git
synced 2026-10-29 19:08:43 +00:00
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
This commit is contained in:
@@ -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<Session>,
|
||||
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<GuardianAssessment, GuardianReviewError> {
|
||||
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)
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
@@ -203,7 +203,7 @@ impl PreparedGuardianContext {
|
||||
}
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(super) struct PreparedReview {
|
||||
pub(in crate::guardian) struct PreparedReview {
|
||||
context: Arc<PreparedGuardianContext>,
|
||||
params: Arc<GuardianReviewSessionParams>,
|
||||
recovery_requested: Arc<AtomicBool>,
|
||||
@@ -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<PreparedReview> {
|
||||
let context = PreparedGuardianContext::prepare(
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -92,43 +92,40 @@ pub enum GuardianReviewSessionOutcome {
|
||||
StaleAuthorization,
|
||||
}
|
||||
|
||||
impl From<GuardianReviewSessionOutcome> 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<GuardianAssessment, GuardianReviewError> {
|
||||
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<GuardianReviewSessionOutcome> for GuardianReviewOutcome {
|
||||
fn from(outcome: GuardianReviewSessionOutcome) -> Self {
|
||||
match outcome.into_assessment() {
|
||||
Ok(assessment) => Self::Completed(assessment),
|
||||
Err(error) => Self::Error(error),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user