mirror of
https://github.com/openai/codex.git
synced 2026-09-09 15:58:47 +00:00
[codex-analytics] guardian review thread and token metadata
This commit is contained in:
@@ -94,6 +94,8 @@ use prompt::render_guardian_transcript_entries;
|
||||
#[cfg(test)]
|
||||
use review::GuardianReviewOutcome;
|
||||
#[cfg(test)]
|
||||
use review::GuardianReviewOutcomeKind;
|
||||
#[cfg(test)]
|
||||
use review::run_guardian_review_session as run_guardian_review_session_for_test;
|
||||
#[cfg(test)]
|
||||
use review_session::build_guardian_review_session_config as build_guardian_review_session_config_for_test;
|
||||
|
||||
@@ -37,6 +37,7 @@ use super::approval_request::guardian_request_target_item_id;
|
||||
use super::approval_request::guardian_request_turn_id;
|
||||
use super::prompt::guardian_output_schema;
|
||||
use super::prompt::parse_guardian_assessment;
|
||||
use super::review_session::GuardianReviewSessionMetadata;
|
||||
use super::review_session::GuardianReviewSessionOutcome;
|
||||
use super::review_session::GuardianReviewSessionParams;
|
||||
use super::review_session::build_guardian_review_session_config;
|
||||
@@ -85,13 +86,55 @@ pub(crate) fn guardian_timeout_message() -> String {
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(super) enum GuardianReviewOutcome {
|
||||
pub(super) struct GuardianReviewOutcome {
|
||||
pub(super) kind: GuardianReviewOutcomeKind,
|
||||
pub(super) metadata: Option<GuardianReviewSessionMetadata>,
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(super) enum GuardianReviewOutcomeKind {
|
||||
Completed(anyhow::Result<GuardianAssessment>),
|
||||
Failed(GuardianReviewFailure),
|
||||
TimedOut,
|
||||
Aborted,
|
||||
}
|
||||
|
||||
impl GuardianReviewOutcome {
|
||||
fn completed(
|
||||
assessment: anyhow::Result<GuardianAssessment>,
|
||||
metadata: Option<GuardianReviewSessionMetadata>,
|
||||
) -> Self {
|
||||
Self {
|
||||
kind: GuardianReviewOutcomeKind::Completed(assessment),
|
||||
metadata,
|
||||
}
|
||||
}
|
||||
|
||||
fn failed(
|
||||
failure: GuardianReviewFailure,
|
||||
metadata: Option<GuardianReviewSessionMetadata>,
|
||||
) -> Self {
|
||||
Self {
|
||||
kind: GuardianReviewOutcomeKind::Failed(failure),
|
||||
metadata,
|
||||
}
|
||||
}
|
||||
|
||||
fn timed_out(metadata: Option<GuardianReviewSessionMetadata>) -> Self {
|
||||
Self {
|
||||
kind: GuardianReviewOutcomeKind::TimedOut,
|
||||
metadata,
|
||||
}
|
||||
}
|
||||
|
||||
fn aborted(metadata: Option<GuardianReviewSessionMetadata>) -> Self {
|
||||
Self {
|
||||
kind: GuardianReviewOutcomeKind::Aborted,
|
||||
metadata,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug)]
|
||||
pub(super) enum GuardianReviewFailure {
|
||||
PromptBuild(anyhow::Error),
|
||||
@@ -208,37 +251,39 @@ struct GuardianReviewAnalyticsResult {
|
||||
completed_at: u64,
|
||||
}
|
||||
|
||||
#[derive(Default)]
|
||||
struct GuardianReviewMetadataFields {
|
||||
guardian_thread_id: Option<String>,
|
||||
guardian_session_kind: Option<GuardianReviewSessionKind>,
|
||||
guardian_model: Option<String>,
|
||||
guardian_reasoning_effort: Option<String>,
|
||||
had_prior_review_context: Option<bool>,
|
||||
reviewed_action_truncated: bool,
|
||||
token_usage: Option<TokenUsage>,
|
||||
time_to_first_token_ms: Option<u64>,
|
||||
}
|
||||
|
||||
impl GuardianReviewAnalyticsResult {
|
||||
fn from_metadata(metadata: GuardianReviewMetadataFields, completed_at: u64) -> Self {
|
||||
Self {
|
||||
fn from_session_metadata(
|
||||
metadata: Option<GuardianReviewSessionMetadata>,
|
||||
completed_at: u64,
|
||||
) -> Self {
|
||||
let mut terminal = Self {
|
||||
decision: GuardianReviewDecision::Denied,
|
||||
terminal_status: GuardianReviewTerminalStatus::FailedClosed,
|
||||
failure_reason: None,
|
||||
risk_level: None,
|
||||
user_authorization: None,
|
||||
outcome: None,
|
||||
guardian_thread_id: metadata.guardian_thread_id,
|
||||
guardian_session_kind: metadata.guardian_session_kind,
|
||||
guardian_model: metadata.guardian_model,
|
||||
guardian_reasoning_effort: metadata.guardian_reasoning_effort,
|
||||
had_prior_review_context: metadata.had_prior_review_context,
|
||||
reviewed_action_truncated: metadata.reviewed_action_truncated,
|
||||
token_usage: metadata.token_usage,
|
||||
time_to_first_token_ms: metadata.time_to_first_token_ms,
|
||||
guardian_thread_id: None,
|
||||
guardian_session_kind: None,
|
||||
guardian_model: None,
|
||||
guardian_reasoning_effort: None,
|
||||
had_prior_review_context: None,
|
||||
reviewed_action_truncated: false,
|
||||
token_usage: None,
|
||||
time_to_first_token_ms: None,
|
||||
completed_at,
|
||||
};
|
||||
|
||||
if let Some(metadata) = metadata {
|
||||
terminal.guardian_thread_id = Some(metadata.guardian_thread_id);
|
||||
terminal.guardian_session_kind = Some(metadata.guardian_session_kind);
|
||||
terminal.guardian_model = Some(metadata.guardian_model);
|
||||
terminal.guardian_reasoning_effort = metadata.guardian_reasoning_effort;
|
||||
terminal.had_prior_review_context = Some(metadata.had_prior_review_context);
|
||||
terminal.token_usage = metadata.token_usage;
|
||||
}
|
||||
|
||||
terminal
|
||||
}
|
||||
}
|
||||
|
||||
@@ -379,10 +424,7 @@ async fn run_guardian_review(
|
||||
decision: GuardianReviewDecision::Aborted,
|
||||
terminal_status: GuardianReviewTerminalStatus::Aborted,
|
||||
failure_reason: Some(GuardianReviewFailureReason::Cancelled),
|
||||
..GuardianReviewAnalyticsResult::from_metadata(
|
||||
GuardianReviewMetadataFields::default(),
|
||||
now_unix_seconds(),
|
||||
)
|
||||
..GuardianReviewAnalyticsResult::from_session_metadata(None, now_unix_seconds())
|
||||
},
|
||||
);
|
||||
session
|
||||
@@ -406,7 +448,7 @@ async fn run_guardian_review(
|
||||
|
||||
let schema = guardian_output_schema();
|
||||
let terminal_action = action_summary.clone();
|
||||
let outcome = Box::pin(run_guardian_review_session(
|
||||
let GuardianReviewOutcome { kind, metadata } = Box::pin(run_guardian_review_session(
|
||||
session.clone(),
|
||||
turn.clone(),
|
||||
request,
|
||||
@@ -417,14 +459,10 @@ async fn run_guardian_review(
|
||||
.await;
|
||||
|
||||
let completed_at = now_unix_seconds();
|
||||
let terminal = || {
|
||||
GuardianReviewAnalyticsResult::from_metadata(
|
||||
GuardianReviewMetadataFields::default(),
|
||||
completed_at,
|
||||
)
|
||||
};
|
||||
let assessment = match outcome {
|
||||
GuardianReviewOutcome::Completed(Ok(assessment)) => {
|
||||
let terminal =
|
||||
|metadata| GuardianReviewAnalyticsResult::from_session_metadata(metadata, completed_at);
|
||||
let assessment = match kind {
|
||||
GuardianReviewOutcomeKind::Completed(Ok(assessment)) => {
|
||||
let approved = matches!(assessment.outcome, GuardianAssessmentOutcome::Allow);
|
||||
analytics_context.track(
|
||||
session.as_ref(),
|
||||
@@ -444,12 +482,12 @@ async fn run_guardian_review(
|
||||
risk_level: Some(assessment.risk_level),
|
||||
user_authorization: Some(assessment.user_authorization),
|
||||
outcome: Some(assessment.outcome),
|
||||
..terminal()
|
||||
..terminal(metadata)
|
||||
},
|
||||
);
|
||||
assessment
|
||||
}
|
||||
GuardianReviewOutcome::Completed(Err(err)) => {
|
||||
GuardianReviewOutcomeKind::Completed(Err(err)) => {
|
||||
let rationale = format!("Automatic approval review failed: {err}");
|
||||
analytics_context.track(
|
||||
session.as_ref(),
|
||||
@@ -458,7 +496,7 @@ async fn run_guardian_review(
|
||||
decision: GuardianReviewDecision::Denied,
|
||||
terminal_status: GuardianReviewTerminalStatus::FailedClosed,
|
||||
failure_reason: Some(GuardianReviewFailureReason::SessionError),
|
||||
..terminal()
|
||||
..terminal(metadata)
|
||||
},
|
||||
);
|
||||
GuardianAssessment {
|
||||
@@ -468,7 +506,7 @@ async fn run_guardian_review(
|
||||
rationale,
|
||||
}
|
||||
}
|
||||
GuardianReviewOutcome::Failed(failure) => {
|
||||
GuardianReviewOutcomeKind::Failed(failure) => {
|
||||
let rationale = format!("Automatic approval review failed: {}", failure.error());
|
||||
analytics_context.track(
|
||||
session.as_ref(),
|
||||
@@ -477,7 +515,7 @@ async fn run_guardian_review(
|
||||
decision: GuardianReviewDecision::Denied,
|
||||
terminal_status: GuardianReviewTerminalStatus::FailedClosed,
|
||||
failure_reason: Some(failure.reason()),
|
||||
..terminal()
|
||||
..terminal(metadata)
|
||||
},
|
||||
);
|
||||
GuardianAssessment {
|
||||
@@ -487,7 +525,7 @@ async fn run_guardian_review(
|
||||
rationale,
|
||||
}
|
||||
}
|
||||
GuardianReviewOutcome::TimedOut => {
|
||||
GuardianReviewOutcomeKind::TimedOut => {
|
||||
let rationale =
|
||||
"Automatic approval review timed out while evaluating the requested approval."
|
||||
.to_string();
|
||||
@@ -498,7 +536,7 @@ async fn run_guardian_review(
|
||||
decision: GuardianReviewDecision::Denied,
|
||||
terminal_status: GuardianReviewTerminalStatus::TimedOut,
|
||||
failure_reason: Some(GuardianReviewFailureReason::Timeout),
|
||||
..terminal()
|
||||
..terminal(metadata)
|
||||
},
|
||||
);
|
||||
session
|
||||
@@ -527,7 +565,7 @@ async fn run_guardian_review(
|
||||
.await;
|
||||
return ReviewDecision::TimedOut;
|
||||
}
|
||||
GuardianReviewOutcome::Aborted => {
|
||||
GuardianReviewOutcomeKind::Aborted => {
|
||||
analytics_context.track(
|
||||
session.as_ref(),
|
||||
turn.as_ref(),
|
||||
@@ -535,7 +573,7 @@ async fn run_guardian_review(
|
||||
decision: GuardianReviewDecision::Aborted,
|
||||
terminal_status: GuardianReviewTerminalStatus::Aborted,
|
||||
failure_reason: Some(GuardianReviewFailureReason::Cancelled),
|
||||
..terminal()
|
||||
..terminal(metadata)
|
||||
},
|
||||
);
|
||||
session
|
||||
@@ -690,7 +728,10 @@ pub(super) async fn run_guardian_review_session(
|
||||
Some(network_proxy) => match network_proxy.proxy().current_cfg().await {
|
||||
Ok(config) => Some(config),
|
||||
Err(err) => {
|
||||
return GuardianReviewOutcome::Failed(GuardianReviewFailure::PromptBuild(err));
|
||||
return GuardianReviewOutcome::failed(
|
||||
GuardianReviewFailure::PromptBuild(err),
|
||||
None,
|
||||
);
|
||||
}
|
||||
},
|
||||
None => None,
|
||||
@@ -741,45 +782,59 @@ pub(super) async fn run_guardian_review_session(
|
||||
);
|
||||
let guardian_config = match guardian_config {
|
||||
Ok(config) => config,
|
||||
Err(err) => return GuardianReviewOutcome::Failed(GuardianReviewFailure::PromptBuild(err)),
|
||||
Err(err) => {
|
||||
return GuardianReviewOutcome::failed(GuardianReviewFailure::PromptBuild(err), None);
|
||||
}
|
||||
};
|
||||
|
||||
match Box::pin(
|
||||
session
|
||||
.guardian_review_session
|
||||
.run_review(GuardianReviewSessionParams {
|
||||
parent_session: Arc::clone(&session),
|
||||
parent_turn: turn.clone(),
|
||||
spawn_config: guardian_config,
|
||||
request,
|
||||
retry_reason,
|
||||
schema,
|
||||
model: guardian_model,
|
||||
reasoning_effort: guardian_reasoning_effort,
|
||||
reasoning_summary: turn.reasoning_summary,
|
||||
personality: turn.personality,
|
||||
external_cancel,
|
||||
}),
|
||||
)
|
||||
.await
|
||||
{
|
||||
let (session_outcome, session_metadata) = Box::pin(session.guardian_review_session.run_review(
|
||||
GuardianReviewSessionParams {
|
||||
parent_session: Arc::clone(&session),
|
||||
parent_turn: turn.clone(),
|
||||
spawn_config: guardian_config,
|
||||
request,
|
||||
retry_reason,
|
||||
schema,
|
||||
model: guardian_model,
|
||||
reasoning_effort: guardian_reasoning_effort,
|
||||
reasoning_summary: turn.reasoning_summary,
|
||||
personality: turn.personality,
|
||||
external_cancel,
|
||||
},
|
||||
))
|
||||
.await;
|
||||
|
||||
match session_outcome {
|
||||
GuardianReviewSessionOutcome::Completed(Ok(last_agent_message)) => match last_agent_message
|
||||
{
|
||||
Some(last_agent_message) => {
|
||||
match parse_guardian_assessment(Some(&last_agent_message)) {
|
||||
Ok(assessment) => GuardianReviewOutcome::Completed(Ok(assessment)),
|
||||
Err(err) => GuardianReviewOutcome::Failed(GuardianReviewFailure::Parse(err)),
|
||||
Ok(assessment) => {
|
||||
GuardianReviewOutcome::completed(Ok(assessment), session_metadata)
|
||||
}
|
||||
Err(err) => GuardianReviewOutcome::failed(
|
||||
GuardianReviewFailure::Parse(err),
|
||||
session_metadata,
|
||||
),
|
||||
}
|
||||
}
|
||||
None => GuardianReviewOutcome::Failed(GuardianReviewFailure::Session(anyhow::anyhow!(
|
||||
"guardian review completed without an assessment payload"
|
||||
))),
|
||||
None => GuardianReviewOutcome::failed(
|
||||
GuardianReviewFailure::Session(anyhow::anyhow!(
|
||||
"guardian review completed without an assessment payload"
|
||||
)),
|
||||
session_metadata,
|
||||
),
|
||||
},
|
||||
GuardianReviewSessionOutcome::Completed(Err(err)) => {
|
||||
GuardianReviewOutcome::Failed(GuardianReviewFailure::Session(err))
|
||||
GuardianReviewOutcome::failed(GuardianReviewFailure::Session(err), session_metadata)
|
||||
}
|
||||
GuardianReviewSessionOutcome::TimedOut => GuardianReviewOutcome::TimedOut,
|
||||
GuardianReviewSessionOutcome::Aborted => GuardianReviewOutcome::Aborted,
|
||||
GuardianReviewSessionOutcome::PromptBuildFailed(err) => {
|
||||
GuardianReviewOutcome::failed(GuardianReviewFailure::PromptBuild(err), session_metadata)
|
||||
}
|
||||
GuardianReviewSessionOutcome::TimedOut => {
|
||||
GuardianReviewOutcome::timed_out(session_metadata)
|
||||
}
|
||||
GuardianReviewSessionOutcome::Aborted => GuardianReviewOutcome::aborted(session_metadata),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::anyhow;
|
||||
use codex_analytics::GuardianReviewSessionKind;
|
||||
use codex_protocol::config_types::Personality;
|
||||
use codex_protocol::config_types::ReasoningSummary as ReasoningSummaryConfig;
|
||||
use codex_protocol::models::DeveloperInstructions;
|
||||
@@ -17,6 +18,7 @@ use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::SubAgentSource;
|
||||
use codex_protocol::protocol::TokenUsage;
|
||||
use serde_json::Value;
|
||||
use tokio::sync::Mutex;
|
||||
use tokio_util::sync::CancellationToken;
|
||||
@@ -58,10 +60,21 @@ const GUARDIAN_FOLLOWUP_REVIEW_REMINDER: &str = concat!(
|
||||
#[derive(Debug)]
|
||||
pub(crate) enum GuardianReviewSessionOutcome {
|
||||
Completed(anyhow::Result<Option<String>>),
|
||||
PromptBuildFailed(anyhow::Error),
|
||||
TimedOut,
|
||||
Aborted,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
pub(crate) struct GuardianReviewSessionMetadata {
|
||||
pub(crate) guardian_thread_id: String,
|
||||
pub(crate) guardian_session_kind: GuardianReviewSessionKind,
|
||||
pub(crate) guardian_model: String,
|
||||
pub(crate) guardian_reasoning_effort: Option<String>,
|
||||
pub(crate) had_prior_review_context: bool,
|
||||
pub(crate) token_usage: Option<TokenUsage>,
|
||||
}
|
||||
|
||||
pub(crate) struct GuardianReviewSessionParams {
|
||||
pub(crate) parent_session: Arc<Session>,
|
||||
pub(crate) parent_turn: Arc<TurnContext>,
|
||||
@@ -101,6 +114,21 @@ struct GuardianReviewState {
|
||||
last_committed_fork_snapshot: Option<GuardianReviewForkSnapshot>,
|
||||
}
|
||||
|
||||
fn had_prior_review_context(prompt_mode: &GuardianPromptMode) -> bool {
|
||||
matches!(prompt_mode, GuardianPromptMode::Delta { .. })
|
||||
}
|
||||
|
||||
fn token_usage_delta(start: &TokenUsage, end: &TokenUsage) -> TokenUsage {
|
||||
TokenUsage {
|
||||
input_tokens: (end.input_tokens - start.input_tokens).max(0),
|
||||
cached_input_tokens: (end.cached_input_tokens - start.cached_input_tokens).max(0),
|
||||
output_tokens: (end.output_tokens - start.output_tokens).max(0),
|
||||
reasoning_output_tokens: (end.reasoning_output_tokens - start.reasoning_output_tokens)
|
||||
.max(0),
|
||||
total_tokens: (end.total_tokens - start.total_tokens).max(0),
|
||||
}
|
||||
}
|
||||
|
||||
struct EphemeralReviewCleanup {
|
||||
state: Arc<Mutex<GuardianReviewSessionState>>,
|
||||
review_session: Option<Arc<GuardianReviewSession>>,
|
||||
@@ -267,10 +295,14 @@ impl GuardianReviewSessionManager {
|
||||
pub(crate) async fn run_review(
|
||||
&self,
|
||||
params: GuardianReviewSessionParams,
|
||||
) -> GuardianReviewSessionOutcome {
|
||||
) -> (
|
||||
GuardianReviewSessionOutcome,
|
||||
Option<GuardianReviewSessionMetadata>,
|
||||
) {
|
||||
let deadline = tokio::time::Instant::now() + GUARDIAN_REVIEW_TIMEOUT;
|
||||
let next_reuse_key = GuardianReviewSessionReuseKey::from_spawn_config(¶ms.spawn_config);
|
||||
let mut stale_trunk_to_shutdown = None;
|
||||
let mut spawned_trunk = false;
|
||||
let trunk_candidate = match run_before_review_deadline(
|
||||
deadline,
|
||||
params.external_cancel.as_ref(),
|
||||
@@ -304,16 +336,17 @@ impl GuardianReviewSessionManager {
|
||||
{
|
||||
Ok(Ok(review_session)) => Arc::new(review_session),
|
||||
Ok(Err(err)) => {
|
||||
return GuardianReviewSessionOutcome::Completed(Err(err));
|
||||
return (GuardianReviewSessionOutcome::PromptBuildFailed(err), None);
|
||||
}
|
||||
Err(outcome) => return outcome,
|
||||
Err(outcome) => return (outcome, None),
|
||||
};
|
||||
state.trunk = Some(Arc::clone(&review_session));
|
||||
spawned_trunk = true;
|
||||
}
|
||||
|
||||
state.trunk.as_ref().cloned()
|
||||
}
|
||||
Err(outcome) => return outcome,
|
||||
Err(outcome) => return (outcome, None),
|
||||
};
|
||||
|
||||
if let Some(review_session) = stale_trunk_to_shutdown {
|
||||
@@ -321,9 +354,12 @@ impl GuardianReviewSessionManager {
|
||||
}
|
||||
|
||||
let Some(trunk) = trunk_candidate else {
|
||||
return GuardianReviewSessionOutcome::Completed(Err(anyhow!(
|
||||
"guardian review session was not available after spawn"
|
||||
)));
|
||||
return (
|
||||
GuardianReviewSessionOutcome::Completed(Err(anyhow!(
|
||||
"guardian review session was not available after spawn"
|
||||
))),
|
||||
None,
|
||||
);
|
||||
};
|
||||
|
||||
if trunk.reuse_key != next_reuse_key {
|
||||
@@ -349,20 +385,30 @@ impl GuardianReviewSessionManager {
|
||||
}
|
||||
};
|
||||
|
||||
let (outcome, keep_review_session) =
|
||||
Box::pin(run_review_on_session(trunk.as_ref(), ¶ms, deadline)).await;
|
||||
let guardian_session_kind = if spawned_trunk {
|
||||
GuardianReviewSessionKind::TrunkNew
|
||||
} else {
|
||||
GuardianReviewSessionKind::TrunkReused
|
||||
};
|
||||
let (outcome, keep_review_session, metadata) = Box::pin(run_review_on_session(
|
||||
trunk.as_ref(),
|
||||
¶ms,
|
||||
guardian_session_kind,
|
||||
deadline,
|
||||
))
|
||||
.await;
|
||||
if keep_review_session && matches!(outcome, GuardianReviewSessionOutcome::Completed(_)) {
|
||||
trunk.refresh_last_committed_fork_snapshot().await;
|
||||
}
|
||||
drop(trunk_guard);
|
||||
|
||||
if keep_review_session {
|
||||
outcome
|
||||
(outcome, Some(metadata))
|
||||
} else {
|
||||
if let Some(review_session) = self.remove_trunk_if_current(&trunk).await {
|
||||
review_session.shutdown_in_background();
|
||||
}
|
||||
outcome
|
||||
(outcome, Some(metadata))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -459,7 +505,10 @@ impl GuardianReviewSessionManager {
|
||||
reuse_key: GuardianReviewSessionReuseKey,
|
||||
deadline: tokio::time::Instant,
|
||||
fork_snapshot: Option<GuardianReviewForkSnapshot>,
|
||||
) -> GuardianReviewSessionOutcome {
|
||||
) -> (
|
||||
GuardianReviewSessionOutcome,
|
||||
Option<GuardianReviewSessionMetadata>,
|
||||
) {
|
||||
let spawn_cancel_token = CancellationToken::new();
|
||||
let mut fork_config = params.spawn_config.clone();
|
||||
fork_config.ephemeral = true;
|
||||
@@ -478,17 +527,18 @@ impl GuardianReviewSessionManager {
|
||||
.await
|
||||
{
|
||||
Ok(Ok(review_session)) => Arc::new(review_session),
|
||||
Ok(Err(err)) => return GuardianReviewSessionOutcome::Completed(Err(err)),
|
||||
Err(outcome) => return outcome,
|
||||
Ok(Err(err)) => return (GuardianReviewSessionOutcome::PromptBuildFailed(err), None),
|
||||
Err(outcome) => return (outcome, None),
|
||||
};
|
||||
self.register_active_ephemeral(Arc::clone(&review_session))
|
||||
.await;
|
||||
let mut cleanup =
|
||||
EphemeralReviewCleanup::new(Arc::clone(&self.state), Arc::clone(&review_session));
|
||||
|
||||
let (outcome, _) = Box::pin(run_review_on_session(
|
||||
let (outcome, _, metadata) = Box::pin(run_review_on_session(
|
||||
review_session.as_ref(),
|
||||
¶ms,
|
||||
GuardianReviewSessionKind::EphemeralForked,
|
||||
deadline,
|
||||
))
|
||||
.await;
|
||||
@@ -496,7 +546,7 @@ impl GuardianReviewSessionManager {
|
||||
cleanup.disarm();
|
||||
review_session.shutdown_in_background();
|
||||
}
|
||||
outcome
|
||||
(outcome, Some(metadata))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -543,8 +593,13 @@ async fn spawn_guardian_review_session(
|
||||
async fn run_review_on_session(
|
||||
review_session: &GuardianReviewSession,
|
||||
params: &GuardianReviewSessionParams,
|
||||
guardian_session_kind: GuardianReviewSessionKind,
|
||||
deadline: tokio::time::Instant,
|
||||
) -> (GuardianReviewSessionOutcome, bool) {
|
||||
) -> (
|
||||
GuardianReviewSessionOutcome,
|
||||
bool,
|
||||
GuardianReviewSessionMetadata,
|
||||
) {
|
||||
let (send_followup_reminder, prompt_mode) = {
|
||||
let state = review_session.state.lock().await;
|
||||
|
||||
@@ -559,6 +614,14 @@ async fn run_review_on_session(
|
||||
|
||||
(send_followup_reminder, prompt_mode)
|
||||
};
|
||||
let mut guardian_metadata = GuardianReviewSessionMetadata {
|
||||
guardian_thread_id: review_session.codex.session.conversation_id.to_string(),
|
||||
guardian_session_kind,
|
||||
guardian_model: params.model.clone(),
|
||||
guardian_reasoning_effort: params.reasoning_effort.map(|effort| effort.to_string()),
|
||||
had_prior_review_context: had_prior_review_context(&prompt_mode),
|
||||
token_usage: None,
|
||||
};
|
||||
if send_followup_reminder {
|
||||
append_guardian_followup_reminder(review_session).await;
|
||||
}
|
||||
@@ -583,6 +646,8 @@ async fn run_review_on_session(
|
||||
prompt_mode,
|
||||
)
|
||||
.await?;
|
||||
let token_usage_at_review_start =
|
||||
review_session.codex.session.total_token_usage().await;
|
||||
|
||||
review_session
|
||||
.codex
|
||||
@@ -602,29 +667,45 @@ async fn run_review_on_session(
|
||||
})
|
||||
.await?;
|
||||
|
||||
Ok::<GuardianTranscriptCursor, anyhow::Error>(prompt_items.transcript_cursor)
|
||||
Ok::<(GuardianTranscriptCursor, Option<TokenUsage>), anyhow::Error>((
|
||||
prompt_items.transcript_cursor,
|
||||
token_usage_at_review_start,
|
||||
))
|
||||
}),
|
||||
)
|
||||
.await;
|
||||
let submit_result = match submit_result {
|
||||
Ok(submit_result) => submit_result,
|
||||
Err(outcome) => return (outcome, false),
|
||||
Err(outcome) => return (outcome, false, guardian_metadata),
|
||||
};
|
||||
let transcript_cursor = match submit_result {
|
||||
Ok(transcript_cursor) => transcript_cursor,
|
||||
let (transcript_cursor, token_usage_at_review_start) = match submit_result {
|
||||
Ok(submit_result) => submit_result,
|
||||
Err(err) => {
|
||||
return (GuardianReviewSessionOutcome::Completed(Err(err)), false);
|
||||
return (
|
||||
GuardianReviewSessionOutcome::PromptBuildFailed(err),
|
||||
false,
|
||||
guardian_metadata,
|
||||
);
|
||||
}
|
||||
};
|
||||
|
||||
let outcome =
|
||||
wait_for_guardian_review(review_session, deadline, params.external_cancel.as_ref()).await;
|
||||
if matches!(outcome.0, GuardianReviewSessionOutcome::Completed(_)) {
|
||||
if outcome.2
|
||||
&& let Some(token_usage_at_review_start) = token_usage_at_review_start
|
||||
&& let Some(total_token_usage) = review_session.codex.session.total_token_usage().await
|
||||
{
|
||||
guardian_metadata.token_usage = Some(token_usage_delta(
|
||||
&token_usage_at_review_start,
|
||||
&total_token_usage,
|
||||
));
|
||||
}
|
||||
let mut state = review_session.state.lock().await;
|
||||
state.prior_review_count = state.prior_review_count.saturating_add(1);
|
||||
state.last_reviewed_transcript_cursor = Some(transcript_cursor);
|
||||
}
|
||||
outcome
|
||||
(outcome.0, outcome.1, guardian_metadata)
|
||||
}
|
||||
|
||||
async fn append_guardian_followup_reminder(review_session: &GuardianReviewSession) {
|
||||
@@ -653,7 +734,7 @@ async fn wait_for_guardian_review(
|
||||
review_session: &GuardianReviewSession,
|
||||
deadline: tokio::time::Instant,
|
||||
external_cancel: Option<&CancellationToken>,
|
||||
) -> (GuardianReviewSessionOutcome, bool) {
|
||||
) -> (GuardianReviewSessionOutcome, bool, bool) {
|
||||
let timeout = tokio::time::sleep_until(deadline);
|
||||
tokio::pin!(timeout);
|
||||
let mut last_error_message: Option<String> = None;
|
||||
@@ -662,7 +743,7 @@ async fn wait_for_guardian_review(
|
||||
tokio::select! {
|
||||
_ = &mut timeout => {
|
||||
let keep_review_session = interrupt_and_drain_turn(&review_session.codex).await.is_ok();
|
||||
return (GuardianReviewSessionOutcome::TimedOut, keep_review_session);
|
||||
return (GuardianReviewSessionOutcome::TimedOut, keep_review_session, false);
|
||||
}
|
||||
_ = async {
|
||||
if let Some(cancel_token) = external_cancel {
|
||||
@@ -672,7 +753,7 @@ async fn wait_for_guardian_review(
|
||||
}
|
||||
} => {
|
||||
let keep_review_session = interrupt_and_drain_turn(&review_session.codex).await.is_ok();
|
||||
return (GuardianReviewSessionOutcome::Aborted, keep_review_session);
|
||||
return (GuardianReviewSessionOutcome::Aborted, keep_review_session, false);
|
||||
}
|
||||
event = review_session.codex.next_event() => {
|
||||
match event {
|
||||
@@ -684,18 +765,20 @@ async fn wait_for_guardian_review(
|
||||
return (
|
||||
GuardianReviewSessionOutcome::Completed(Err(anyhow!(error_message))),
|
||||
true,
|
||||
true,
|
||||
);
|
||||
}
|
||||
return (
|
||||
GuardianReviewSessionOutcome::Completed(Ok(turn_complete.last_agent_message)),
|
||||
true,
|
||||
true,
|
||||
);
|
||||
}
|
||||
EventMsg::Error(error) => {
|
||||
last_error_message = Some(error.message);
|
||||
}
|
||||
EventMsg::TurnAborted(_) => {
|
||||
return (GuardianReviewSessionOutcome::Aborted, true);
|
||||
return (GuardianReviewSessionOutcome::Aborted, true, false);
|
||||
}
|
||||
_ => {}
|
||||
},
|
||||
@@ -703,6 +786,7 @@ async fn wait_for_guardian_review(
|
||||
return (
|
||||
GuardianReviewSessionOutcome::Completed(Err(err.into())),
|
||||
false,
|
||||
false,
|
||||
);
|
||||
}
|
||||
}
|
||||
@@ -954,4 +1038,44 @@ mod tests {
|
||||
assert_eq!(outcome.unwrap(), 42);
|
||||
assert!(!cancel_token.is_cancelled());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn had_prior_review_context_tracks_prompt_mode() {
|
||||
assert!(!had_prior_review_context(&GuardianPromptMode::Full));
|
||||
assert!(had_prior_review_context(&GuardianPromptMode::Delta {
|
||||
cursor: GuardianTranscriptCursor {
|
||||
parent_history_version: 7,
|
||||
transcript_entry_count: 42,
|
||||
}
|
||||
}));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn token_usage_delta_never_reports_negative_usage() {
|
||||
let start = TokenUsage {
|
||||
input_tokens: 10,
|
||||
cached_input_tokens: 8,
|
||||
output_tokens: 6,
|
||||
reasoning_output_tokens: 4,
|
||||
total_tokens: 28,
|
||||
};
|
||||
let end = TokenUsage {
|
||||
input_tokens: 15,
|
||||
cached_input_tokens: 7,
|
||||
output_tokens: 10,
|
||||
reasoning_output_tokens: 2,
|
||||
total_tokens: 34,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
token_usage_delta(&start, &end),
|
||||
TokenUsage {
|
||||
input_tokens: 5,
|
||||
cached_input_tokens: 0,
|
||||
output_tokens: 4,
|
||||
reasoning_output_tokens: 0,
|
||||
total_tokens: 6,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -15,6 +15,7 @@ use crate::config_loader::NetworkDomainPermissionsToml;
|
||||
use crate::config_loader::RequirementSource;
|
||||
use crate::config_loader::Sourced;
|
||||
use crate::test_support;
|
||||
use codex_analytics::GuardianApprovalRequestSource;
|
||||
use codex_config::config_toml::ConfigToml;
|
||||
use codex_network_proxy::NetworkProxyConfig;
|
||||
use codex_protocol::ThreadId;
|
||||
@@ -917,7 +918,11 @@ async fn guardian_review_request_layout_matches_model_visible_request_snapshot()
|
||||
/*external_cancel*/ None,
|
||||
)
|
||||
.await;
|
||||
let GuardianReviewOutcome::Completed(Ok(assessment)) = outcome else {
|
||||
let GuardianReviewOutcome {
|
||||
kind: GuardianReviewOutcomeKind::Completed(Ok(assessment)),
|
||||
..
|
||||
} = outcome
|
||||
else {
|
||||
panic!("expected guardian assessment");
|
||||
};
|
||||
assert_eq!(assessment.outcome, GuardianAssessmentOutcome::Allow);
|
||||
@@ -1126,13 +1131,25 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow:
|
||||
)
|
||||
.await;
|
||||
|
||||
let GuardianReviewOutcome::Completed(Ok(first_assessment)) = first_outcome else {
|
||||
let GuardianReviewOutcome {
|
||||
kind: GuardianReviewOutcomeKind::Completed(Ok(first_assessment)),
|
||||
..
|
||||
} = first_outcome
|
||||
else {
|
||||
panic!("expected first guardian assessment");
|
||||
};
|
||||
let GuardianReviewOutcome::Completed(Ok(second_assessment)) = second_outcome else {
|
||||
let GuardianReviewOutcome {
|
||||
kind: GuardianReviewOutcomeKind::Completed(Ok(second_assessment)),
|
||||
..
|
||||
} = second_outcome
|
||||
else {
|
||||
panic!("expected second guardian assessment");
|
||||
};
|
||||
let GuardianReviewOutcome::Completed(Ok(third_assessment)) = third_outcome else {
|
||||
let GuardianReviewOutcome {
|
||||
kind: GuardianReviewOutcomeKind::Completed(Ok(third_assessment)),
|
||||
..
|
||||
} = third_outcome
|
||||
else {
|
||||
panic!("expected third guardian assessment");
|
||||
};
|
||||
assert_eq!(first_assessment.outcome, GuardianAssessmentOutcome::Allow);
|
||||
|
||||
Reference in New Issue
Block a user