diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index 880adfc254fc..0709838a9e5a 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -9,18 +9,25 @@ use crate::events::CodexCompactionEventRequest; use crate::events::CodexHookRunEventRequest; use crate::events::CodexPluginEventRequest; use crate::events::CodexPluginUsedEventRequest; +use crate::events::CodexReviewEventParams; +use crate::events::CodexReviewEventRequest; use crate::events::CodexRuntimeMetadata; use crate::events::CodexToolItemEventBase; use crate::events::CodexTurnEventRequest; +use crate::events::FinalApprovalOutcome; use crate::events::GuardianApprovalRequestSource; use crate::events::GuardianReviewDecision; use crate::events::GuardianReviewEventParams; use crate::events::GuardianReviewFailureReason; use crate::events::GuardianReviewTerminalStatus; use crate::events::GuardianReviewedAction; +use crate::events::ReviewResolution; +use crate::events::ReviewStatus; +use crate::events::ReviewSubjectKind; +use crate::events::ReviewTrigger; +use crate::events::Reviewer; use crate::events::ThreadInitializedEvent; use crate::events::ThreadInitializedEventParams; -use crate::events::ToolItemFinalApprovalOutcome; use crate::events::ToolItemTerminalStatus; use crate::events::TrackEventRequest; use crate::events::codex_app_metadata; @@ -67,17 +74,32 @@ use codex_app_server_protocol::ClientRequest; use codex_app_server_protocol::ClientResponsePayload; use codex_app_server_protocol::CodexErrorInfo; use codex_app_server_protocol::CommandAction; +use codex_app_server_protocol::CommandExecutionApprovalDecision; +use codex_app_server_protocol::CommandExecutionRequestApprovalParams; +use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionSource; use codex_app_server_protocol::CommandExecutionStatus; +use codex_app_server_protocol::GrantedPermissionProfile; +use codex_app_server_protocol::GuardianApprovalReview; +use codex_app_server_protocol::GuardianApprovalReviewAction; +use codex_app_server_protocol::GuardianApprovalReviewStatus; +use codex_app_server_protocol::GuardianCommandSource as AppServerGuardianCommandSource; use codex_app_server_protocol::InitializeCapabilities; use codex_app_server_protocol::InitializeParams; use codex_app_server_protocol::ItemCompletedNotification; +use codex_app_server_protocol::ItemGuardianApprovalReviewCompletedNotification; use codex_app_server_protocol::ItemStartedNotification; use codex_app_server_protocol::JSONRPCErrorError; use codex_app_server_protocol::NonSteerableTurnKind; +use codex_app_server_protocol::PermissionGrantScope; +use codex_app_server_protocol::PermissionsRequestApprovalParams; +use codex_app_server_protocol::PermissionsRequestApprovalResponse; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::RequestPermissionProfile; use codex_app_server_protocol::SandboxPolicy as AppServerSandboxPolicy; use codex_app_server_protocol::ServerNotification; +use codex_app_server_protocol::ServerRequest; +use codex_app_server_protocol::ServerResponse; use codex_app_server_protocol::SessionSource as AppServerSessionSource; use codex_app_server_protocol::Thread; use codex_app_server_protocol::ThreadArchiveParams; @@ -603,7 +625,7 @@ async fn ingest_turn_prerequisites( } } -async fn ingest_tool_review_prerequisites( +async fn ingest_review_prerequisites( reducer: &mut AnalyticsReducer, events: &mut Vec, ) { @@ -625,6 +647,50 @@ async fn ingest_tool_review_prerequisites( events.clear(); } +async fn ingest_completed_command_execution_item( + reducer: &mut AnalyticsReducer, + events: &mut Vec, + thread_id: &str, + item_id: &str, +) { + reducer + .ingest( + AnalyticsFact::Notification(Box::new(ServerNotification::ItemStarted( + ItemStartedNotification { + thread_id: thread_id.to_string(), + turn_id: "turn-1".to_string(), + started_at_ms: 1_000, + item: sample_command_execution_item_with_id( + item_id, + CommandExecutionStatus::InProgress, + /*exit_code*/ None, + /*duration_ms*/ None, + ), + }, + ))), + events, + ) + .await; + reducer + .ingest( + AnalyticsFact::Notification(Box::new(ServerNotification::ItemCompleted( + ItemCompletedNotification { + thread_id: thread_id.to_string(), + turn_id: "turn-1".to_string(), + completed_at_ms: 1_042, + item: sample_command_execution_item_with_id( + item_id, + CommandExecutionStatus::Completed, + Some(0), + Some(42), + ), + }, + ))), + events, + ) + .await; +} + fn sample_initialize_fact(connection_id: u64) -> AnalyticsFact { AnalyticsFact::Initialize { connection_id, @@ -654,9 +720,18 @@ fn sample_command_execution_item( status: CommandExecutionStatus, exit_code: Option, duration_ms: Option, +) -> ThreadItem { + sample_command_execution_item_with_id("item-1", status, exit_code, duration_ms) +} + +fn sample_command_execution_item_with_id( + id: &str, + status: CommandExecutionStatus, + exit_code: Option, + duration_ms: Option, ) -> ThreadItem { ThreadItem::CommandExecution { - id: "item-1".to_string(), + id: id.to_string(), command: "echo hi".to_string(), cwd: test_path_buf("/tmp").abs(), process_id: Some("pid-1".to_string()), @@ -687,6 +762,106 @@ fn sample_command_execution_item_with_actions( item } +fn sample_command_approval_request( + request_id: i64, + approval_id: Option<&str>, + source: AppServerGuardianCommandSource, +) -> ServerRequest { + ServerRequest::CommandExecutionRequestApproval { + request_id: RequestId::Integer(request_id), + params: CommandExecutionRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "item-1".to_string(), + started_at_ms: 1_000, + approval_id: approval_id.map(str::to_string), + source, + reason: None, + network_approval_context: None, + command: Some("echo hi".to_string()), + cwd: None, + command_actions: None, + additional_permissions: None, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments: None, + available_decisions: None, + }, + } +} + +fn sample_command_approval_response( + request_id: i64, + decision: CommandExecutionApprovalDecision, +) -> ServerResponse { + ServerResponse::CommandExecutionRequestApproval { + request_id: RequestId::Integer(request_id), + response: CommandExecutionRequestApprovalResponse { decision }, + } +} + +fn sample_permissions_approval_request(request_id: i64) -> ServerRequest { + ServerRequest::PermissionsRequestApproval { + request_id: RequestId::Integer(request_id), + params: PermissionsRequestApprovalParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: "permissions-1".to_string(), + started_at_ms: 1_000, + cwd: test_path_buf("/tmp").abs(), + reason: Some("need network".to_string()), + permissions: RequestPermissionProfile { + network: Some(codex_app_server_protocol::AdditionalNetworkPermissions { + enabled: Some(true), + }), + file_system: None, + }, + }, + } +} + +fn sample_permissions_approval_response( + request_id: i64, + permissions: GrantedPermissionProfile, +) -> ServerResponse { + ServerResponse::PermissionsRequestApproval { + request_id: RequestId::Integer(request_id), + response: PermissionsRequestApprovalResponse { + permissions, + scope: PermissionGrantScope::Turn, + strict_auto_review: None, + }, + } +} + +fn sample_guardian_review_completed( + review_id: &str, + target_item_id: Option<&str>, + status: GuardianApprovalReviewStatus, +) -> ServerNotification { + ServerNotification::ItemGuardianApprovalReviewCompleted( + ItemGuardianApprovalReviewCompletedNotification { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + started_at_ms: 1_000, + completed_at_ms: 1_042, + review_id: review_id.to_string(), + target_item_id: target_item_id.map(str::to_string), + decision_source: codex_app_server_protocol::AutoReviewDecisionSource::Agent, + review: GuardianApprovalReview { + status, + risk_level: None, + user_authorization: None, + rationale: None, + }, + action: GuardianApprovalReviewAction::Command { + source: AppServerGuardianCommandSource::Shell, + command: "echo hi".to_string(), + cwd: test_path_buf("/tmp").abs(), + }, + }, + ) +} + fn expected_absolute_path(path: &PathBuf) -> String { std::fs::canonicalize(path) .unwrap_or_else(|_| path.to_path_buf()) @@ -1023,7 +1198,7 @@ fn command_execution_event_serializes_expected_shape() { review_count: 0, guardian_review_count: 0, user_review_count: 0, - final_approval_outcome: ToolItemFinalApprovalOutcome::NotNeeded, + final_approval_outcome: FinalApprovalOutcome::NotNeeded, terminal_status: ToolItemTerminalStatus::Completed, failure_kind: None, requested_additional_permissions: false, @@ -1089,6 +1264,82 @@ fn command_execution_event_serializes_expected_shape() { ); } +#[test] +fn review_event_serializes_expected_shape() { + let event = TrackEventRequest::ReviewEvent(CodexReviewEventRequest { + event_type: "codex_review_event", + event_params: CodexReviewEventParams { + thread_id: "thread-1".to_string(), + turn_id: "turn-1".to_string(), + item_id: None, + review_id: "review-1".to_string(), + app_server_client: CodexAppServerClientMetadata { + product_client_id: "codex_tui".to_string(), + client_name: Some("codex-tui".to_string()), + client_version: Some("1.2.3".to_string()), + rpc_transport: AppServerRpcTransport::Websocket, + experimental_api_enabled: Some(true), + }, + runtime: CodexRuntimeMetadata { + codex_rs_version: "0.99.0".to_string(), + runtime_os: "macos".to_string(), + runtime_os_version: "15.3.1".to_string(), + runtime_arch: "aarch64".to_string(), + }, + thread_source: Some(ThreadSource::Subagent), + subagent_source: Some("thread_spawn".to_string()), + parent_thread_id: Some("parent-thread-1".to_string()), + subject_kind: ReviewSubjectKind::NetworkAccess, + subject_name: "network_access".to_string(), + reviewer: Reviewer::User, + trigger: ReviewTrigger::NetworkPolicyDenial, + status: ReviewStatus::Approved, + resolution: ReviewResolution::NetworkPolicyAmendment, + started_at_ms: 123, + completed_at_ms: 125, + duration_ms: Some(2), + }, + }); + + let payload = serde_json::to_value(&event).expect("serialize review event"); + assert_eq!( + payload, + json!({ + "event_type": "codex_review_event", + "event_params": { + "thread_id": "thread-1", + "turn_id": "turn-1", + "item_id": null, + "review_id": "review-1", + "app_server_client": { + "product_client_id": "codex_tui", + "client_name": "codex-tui", + "client_version": "1.2.3", + "rpc_transport": "websocket", + "experimental_api_enabled": true + }, + "runtime": { + "codex_rs_version": "0.99.0", + "runtime_os": "macos", + "runtime_os_version": "15.3.1", + "runtime_arch": "aarch64" + }, + "thread_source": "subagent", + "subagent_source": "thread_spawn", + "parent_thread_id": "parent-thread-1", + "subject_kind": "network_access", + "subject_name": "network_access", + "reviewer": "user", + "trigger": "network_policy_denial", + "status": "approved", + "resolution": "network_policy_amendment", + "started_at_ms": 123, + "completed_at_ms": 125, + "duration_ms": 2 + } + }) + ); +} #[tokio::test] async fn initialize_caches_client_and_thread_lifecycle_publishes_once_initialized() { let mut reducer = AnalyticsReducer::default(); @@ -1500,7 +1751,7 @@ async fn item_lifecycle_notifications_publish_command_execution_event() { let mut reducer = AnalyticsReducer::default(); let mut events = Vec::new(); - ingest_tool_review_prerequisites(&mut reducer, &mut events).await; + ingest_review_prerequisites(&mut reducer, &mut events).await; reducer .ingest( AnalyticsFact::Notification(Box::new(ServerNotification::ItemStarted( @@ -1603,6 +1854,295 @@ async fn item_lifecycle_notifications_publish_command_execution_event() { assert_eq!(payload[0]["event_params"]["thread_source"], "user"); } +#[tokio::test] +async fn command_execution_approval_response_publishes_user_review_event() { + let mut reducer = AnalyticsReducer::default(); + let mut events = Vec::new(); + + ingest_review_prerequisites(&mut reducer, &mut events).await; + reducer + .ingest( + AnalyticsFact::ServerRequest { + connection_id: 7, + request: Box::new(sample_command_approval_request( + /*request_id*/ 41, + /*approval_id*/ None, + AppServerGuardianCommandSource::Shell, + )), + }, + &mut events, + ) + .await; + assert!(events.is_empty()); + + reducer + .ingest( + AnalyticsFact::ServerResponse { + completed_at_ms: 1_042, + response: Box::new(sample_command_approval_response( + /*request_id*/ 41, + CommandExecutionApprovalDecision::Accept, + )), + }, + &mut events, + ) + .await; + + let payload = serde_json::to_value(&events).expect("serialize events"); + assert_eq!(payload.as_array().expect("events array").len(), 1); + assert_eq!(payload[0]["event_type"], "codex_review_event"); + assert_eq!(payload[0]["event_params"]["thread_id"], "thread-1"); + assert_eq!(payload[0]["event_params"]["turn_id"], "turn-1"); + assert_eq!(payload[0]["event_params"]["item_id"], "item-1"); + assert_eq!(payload[0]["event_params"]["review_id"], "user:41"); + assert_eq!(payload[0]["event_params"]["thread_source"], "user"); + assert_eq!( + payload[0]["event_params"]["subject_kind"], + "command_execution" + ); + assert_eq!(payload[0]["event_params"]["subject_name"], "shell"); + assert_eq!(payload[0]["event_params"]["reviewer"], "user"); + assert_eq!(payload[0]["event_params"]["trigger"], "initial"); + assert_eq!(payload[0]["event_params"]["status"], "approved"); + assert_eq!(payload[0]["event_params"]["started_at_ms"], 1_000); + assert_eq!(payload[0]["event_params"]["completed_at_ms"], 1_042); + assert_eq!(payload[0]["event_params"]["duration_ms"], 42); +} + +#[tokio::test] +async fn permissions_reviews_emit_events_without_denormalizing_onto_tool_items() { + let mut reducer = AnalyticsReducer::default(); + let mut events = Vec::new(); + + ingest_review_prerequisites(&mut reducer, &mut events).await; + reducer + .ingest( + AnalyticsFact::ServerRequest { + connection_id: 7, + request: Box::new(sample_permissions_approval_request(/*request_id*/ 51)), + }, + &mut events, + ) + .await; + assert!(events.is_empty()); + + reducer + .ingest( + AnalyticsFact::ServerResponse { + completed_at_ms: 1_042, + response: Box::new(sample_permissions_approval_response( + /*request_id*/ 51, + GrantedPermissionProfile::default(), + )), + }, + &mut events, + ) + .await; + + let payload = serde_json::to_value(&events).expect("serialize events"); + assert_eq!(payload.as_array().expect("events array").len(), 1); + assert_eq!(payload[0]["event_type"], "codex_review_event"); + assert_eq!(payload[0]["event_params"]["review_id"], "user:51"); + assert_eq!(payload[0]["event_params"]["subject_kind"], "permissions"); + assert_eq!(payload[0]["event_params"]["reviewer"], "user"); + assert_eq!(payload[0]["event_params"]["status"], "denied"); + assert_eq!(payload[0]["event_params"]["resolution"], "none"); + + events.clear(); + ingest_completed_command_execution_item(&mut reducer, &mut events, "thread-1", "permissions-1") + .await; + + let payload = serde_json::to_value(&events[0]).expect("serialize tool item event"); + assert_eq!(payload["event_params"]["item_id"], "permissions-1"); + assert_eq!(payload["event_params"]["review_count"], 0); + assert_eq!(payload["event_params"]["user_review_count"], 0); + assert_eq!(payload["event_params"]["guardian_review_count"], 0); +} + +#[tokio::test] +async fn aborted_server_request_publishes_aborted_user_review_event_once() { + let mut reducer = AnalyticsReducer::default(); + let mut events = Vec::new(); + + ingest_review_prerequisites(&mut reducer, &mut events).await; + reducer + .ingest( + AnalyticsFact::ServerRequest { + connection_id: 7, + request: Box::new(sample_command_approval_request( + /*request_id*/ 61, + /*approval_id*/ None, + AppServerGuardianCommandSource::Shell, + )), + }, + &mut events, + ) + .await; + reducer + .ingest( + AnalyticsFact::ServerRequestAborted { + completed_at_ms: 1_042, + request_id: RequestId::Integer(61), + }, + &mut events, + ) + .await; + + let payload = serde_json::to_value(&events).expect("serialize events"); + assert_eq!(payload.as_array().expect("events array").len(), 1); + assert_eq!(payload[0]["event_params"]["review_id"], "user:61"); + assert_eq!(payload[0]["event_params"]["status"], "aborted"); + assert_eq!(payload[0]["event_params"]["resolution"], "none"); + + events.clear(); + reducer + .ingest( + AnalyticsFact::ServerResponse { + completed_at_ms: 1_043, + response: Box::new(sample_command_approval_response( + /*request_id*/ 61, + CommandExecutionApprovalDecision::Accept, + )), + }, + &mut events, + ) + .await; + assert!(events.is_empty()); +} + +#[tokio::test] +async fn guardian_completed_notification_publishes_review_event_with_thread_metadata() { + let mut reducer = AnalyticsReducer::default(); + let mut events = Vec::new(); + + ingest_review_prerequisites(&mut reducer, &mut events).await; + reducer + .ingest( + AnalyticsFact::Notification(Box::new(sample_guardian_review_completed( + "guardian-review-1", + Some("item-1"), + GuardianApprovalReviewStatus::Denied, + ))), + &mut events, + ) + .await; + + let payload = serde_json::to_value(&events[0]).expect("serialize review event"); + assert_eq!(payload["event_type"], "codex_review_event"); + assert_eq!(payload["event_params"]["review_id"], "guardian-review-1"); + assert_eq!(payload["event_params"]["item_id"], "item-1"); + assert_eq!(payload["event_params"]["thread_source"], "user"); + assert_eq!(payload["event_params"]["subject_kind"], "command_execution"); + assert_eq!(payload["event_params"]["reviewer"], "guardian"); + assert_eq!(payload["event_params"]["status"], "denied"); + assert_eq!(payload["event_params"]["started_at_ms"], 1_000); + assert_eq!(payload["event_params"]["completed_at_ms"], 1_042); + assert_eq!(payload["event_params"]["duration_ms"], 42); +} + +#[tokio::test] +async fn terminal_reviews_denormalize_counts_onto_tool_item_events() { + let mut reducer = AnalyticsReducer::default(); + let mut events = Vec::new(); + + ingest_review_prerequisites(&mut reducer, &mut events).await; + reducer + .ingest( + AnalyticsFact::ServerRequest { + connection_id: 7, + request: Box::new(sample_command_approval_request( + /*request_id*/ 71, + /*approval_id*/ None, + AppServerGuardianCommandSource::Shell, + )), + }, + &mut events, + ) + .await; + reducer + .ingest( + AnalyticsFact::ServerResponse { + completed_at_ms: 1_042, + response: Box::new(sample_command_approval_response( + /*request_id*/ 71, + CommandExecutionApprovalDecision::AcceptForSession, + )), + }, + &mut events, + ) + .await; + events.clear(); + + ingest_completed_command_execution_item(&mut reducer, &mut events, "thread-1", "item-1").await; + + let payload = serde_json::to_value(&events[0]).expect("serialize tool item event"); + assert_eq!(payload["event_params"]["review_count"], 1); + assert_eq!(payload["event_params"]["user_review_count"], 1); + assert_eq!(payload["event_params"]["guardian_review_count"], 0); + assert_eq!( + payload["event_params"]["final_approval_outcome"], + "user_approved_for_session" + ); +} + +#[tokio::test] +async fn item_review_summaries_do_not_cross_threads_with_reused_item_ids() { + let mut reducer = AnalyticsReducer::default(); + let mut events = Vec::new(); + + ingest_review_prerequisites(&mut reducer, &mut events).await; + reducer + .ingest( + AnalyticsFact::ClientResponse { + connection_id: 7, + request_id: RequestId::Integer(2), + response: Box::new(sample_thread_start_response( + "thread-2", /*ephemeral*/ false, "gpt-5", + )), + }, + &mut events, + ) + .await; + events.clear(); + + reducer + .ingest( + AnalyticsFact::ServerRequest { + connection_id: 7, + request: Box::new(sample_command_approval_request( + /*request_id*/ 72, + /*approval_id*/ None, + AppServerGuardianCommandSource::Shell, + )), + }, + &mut events, + ) + .await; + reducer + .ingest( + AnalyticsFact::ServerResponse { + completed_at_ms: 1_042, + response: Box::new(sample_command_approval_response( + /*request_id*/ 72, + CommandExecutionApprovalDecision::Accept, + )), + }, + &mut events, + ) + .await; + events.clear(); + + ingest_completed_command_execution_item(&mut reducer, &mut events, "thread-2", "item-1").await; + + let payload = serde_json::to_value(&events[0]).expect("serialize tool item event"); + assert_eq!(payload["event_params"]["thread_id"], "thread-2"); + assert_eq!(payload["event_params"]["item_id"], "item-1"); + assert_eq!(payload["event_params"]["review_count"], 0); + assert_eq!(payload["event_params"]["user_review_count"], 0); + assert_eq!(payload["event_params"]["guardian_review_count"], 0); + assert_eq!(payload["event_params"]["final_approval_outcome"], "unknown"); +} + #[test] fn subagent_thread_started_review_serializes_expected_shape() { let event = TrackEventRequest::ThreadInitialized(subagent_thread_started_event_request( @@ -1891,7 +2431,7 @@ async fn subagent_tool_items_inherit_parent_connection_metadata() { let mut reducer = AnalyticsReducer::default(); let mut events = Vec::new(); - ingest_tool_review_prerequisites(&mut reducer, &mut events).await; + ingest_review_prerequisites(&mut reducer, &mut events).await; reducer .ingest( AnalyticsFact::Custom(CustomAnalyticsFact::SubAgentThreadStarted( diff --git a/codex-rs/analytics/src/client.rs b/codex-rs/analytics/src/client.rs index 6d46b2ce5709..5a1536eca02a 100644 --- a/codex-rs/analytics/src/client.rs +++ b/codex-rs/analytics/src/client.rs @@ -171,9 +171,10 @@ impl AnalyticsEventsClient { &self, tracking: &GuardianReviewTrackContext, result: GuardianReviewAnalyticsResult, + completed_at_ms: u64, ) { self.record_fact(AnalyticsFact::Custom(CustomAnalyticsFact::GuardianReview( - Box::new(tracking.event_params(result)), + Box::new(tracking.event_params(result, completed_at_ms)), ))); } @@ -347,6 +348,13 @@ impl AnalyticsEventsClient { }); } + pub fn track_server_request_aborted(&self, completed_at_ms: u64, request_id: RequestId) { + self.record_fact(AnalyticsFact::ServerRequestAborted { + completed_at_ms, + request_id, + }); + } + pub fn track_notification(&self, notification: ServerNotification) { if !matches!( notification, diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index eaa7daf8f866..1b23e8a67630 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -19,7 +19,6 @@ use crate::facts::TurnSteerRejectionReason; use crate::facts::TurnSteerResult; use crate::facts::TurnSubmissionType; use crate::now_unix_millis; -use crate::now_unix_seconds; use codex_app_server_protocol::CodexErrorInfo; use codex_app_server_protocol::CommandExecutionSource; use codex_login::default_client::originator; @@ -292,6 +291,7 @@ impl GuardianReviewTrackContext { pub(crate) fn event_params( &self, result: GuardianReviewAnalyticsResult, + completed_at_ms: u64, ) -> GuardianReviewEventParams { GuardianReviewEventParams { thread_id: self.thread_id.clone(), @@ -318,7 +318,7 @@ impl GuardianReviewTrackContext { time_to_first_token_ms: result.time_to_first_token_ms, completion_latency_ms: Some(self.started_instant.elapsed().as_millis() as u64), started_at: self.started_at_ms / 1_000, - completed_at: Some(now_unix_seconds()), + completed_at: Some(completed_at_ms / 1_000), input_tokens: result.token_usage.as_ref().map(|usage| usage.input_tokens), cached_input_tokens: result .token_usage @@ -401,7 +401,7 @@ pub(crate) struct GuardianReviewEventPayload { #[allow(dead_code)] #[derive(Clone, Copy, Debug, Serialize)] #[serde(rename_all = "snake_case")] -pub(crate) enum ToolItemFinalApprovalOutcome { +pub(crate) enum FinalApprovalOutcome { Unknown, NotNeeded, ConfigAllowed, @@ -458,7 +458,7 @@ pub(crate) struct CodexToolItemEventBase { pub(crate) review_count: u64, pub(crate) guardian_review_count: u64, pub(crate) user_review_count: u64, - pub(crate) final_approval_outcome: ToolItemFinalApprovalOutcome, + pub(crate) final_approval_outcome: FinalApprovalOutcome, pub(crate) terminal_status: ToolItemTerminalStatus, pub(crate) failure_kind: Option, pub(crate) requested_additional_permissions: bool, @@ -525,8 +525,8 @@ pub(crate) struct CodexReviewEventParams { pub(crate) thread_source: Option, pub(crate) subagent_source: Option, pub(crate) parent_thread_id: Option, - pub(crate) tool_kind: ReviewSubjectKind, - pub(crate) tool_name: String, + pub(crate) subject_kind: ReviewSubjectKind, + pub(crate) subject_name: String, pub(crate) reviewer: Reviewer, pub(crate) trigger: ReviewTrigger, pub(crate) status: ReviewStatus, diff --git a/codex-rs/analytics/src/facts.rs b/codex-rs/analytics/src/facts.rs index d0446e8c0ca2..bb28439c3dc8 100644 --- a/codex-rs/analytics/src/facts.rs +++ b/codex-rs/analytics/src/facts.rs @@ -299,6 +299,10 @@ pub(crate) enum AnalyticsFact { completed_at_ms: u64, response: Box, }, + ServerRequestAborted { + completed_at_ms: u64, + request_id: RequestId, + }, Notification(Box), // Facts that do not naturally exist on the app-server protocol surface, or // would require non-trivial protocol reshaping on this branch. diff --git a/codex-rs/analytics/src/reducer.rs b/codex-rs/analytics/src/reducer.rs index 2ddb59c0cfee..aadb9511fb0b 100644 --- a/codex-rs/analytics/src/reducer.rs +++ b/codex-rs/analytics/src/reducer.rs @@ -18,6 +18,8 @@ use crate::events::CodexMcpToolCallEventParams; use crate::events::CodexMcpToolCallEventRequest; use crate::events::CodexPluginEventRequest; use crate::events::CodexPluginUsedEventRequest; +use crate::events::CodexReviewEventParams; +use crate::events::CodexReviewEventRequest; use crate::events::CodexRuntimeMetadata; use crate::events::CodexToolItemEventBase; use crate::events::CodexTurnEventParams; @@ -26,15 +28,20 @@ use crate::events::CodexTurnSteerEventParams; use crate::events::CodexTurnSteerEventRequest; use crate::events::CodexWebSearchEventParams; use crate::events::CodexWebSearchEventRequest; +use crate::events::FinalApprovalOutcome; use crate::events::GuardianReviewEventParams; use crate::events::GuardianReviewEventPayload; use crate::events::GuardianReviewEventRequest; +use crate::events::ReviewResolution; +use crate::events::ReviewStatus; +use crate::events::ReviewSubjectKind; +use crate::events::ReviewTrigger; +use crate::events::Reviewer; use crate::events::SkillInvocationEventParams; use crate::events::SkillInvocationEventRequest; use crate::events::ThreadInitializedEvent; use crate::events::ThreadInitializedEventParams; use crate::events::ToolItemFailureKind; -use crate::events::ToolItemFinalApprovalOutcome; use crate::events::ToolItemTerminalStatus; use crate::events::TrackEventRequest; use crate::events::WebSearchActionKind; @@ -76,16 +83,26 @@ use codex_app_server_protocol::CollabAgentStatus; use codex_app_server_protocol::CollabAgentTool; use codex_app_server_protocol::CollabAgentToolCallStatus; use codex_app_server_protocol::CommandAction; +use codex_app_server_protocol::CommandExecutionApprovalDecision; use codex_app_server_protocol::CommandExecutionSource; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::DynamicToolCallOutputContentItem; use codex_app_server_protocol::DynamicToolCallStatus; +use codex_app_server_protocol::FileChangeApprovalDecision; +use codex_app_server_protocol::GuardianApprovalReviewAction; +use codex_app_server_protocol::GuardianApprovalReviewStatus; +use codex_app_server_protocol::GuardianCommandSource as AppServerGuardianCommandSource; use codex_app_server_protocol::InitializeParams; use codex_app_server_protocol::McpToolCallStatus; +use codex_app_server_protocol::NetworkPolicyRuleAction; use codex_app_server_protocol::PatchApplyStatus; use codex_app_server_protocol::PatchChangeKind; +use codex_app_server_protocol::PermissionGrantScope; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::RequestPermissionProfile; use codex_app_server_protocol::ServerNotification; +use codex_app_server_protocol::ServerRequest; +use codex_app_server_protocol::ServerResponse; use codex_app_server_protocol::ThreadItem; use codex_app_server_protocol::TurnSteerResponse; use codex_app_server_protocol::UserInput; @@ -112,6 +129,8 @@ pub(crate) struct AnalyticsReducer { connections: HashMap, threads: HashMap, tool_items_started_at_ms: HashMap, + pending_reviews: HashMap, + item_review_summaries: HashMap, } struct ConnectionState { @@ -145,6 +164,16 @@ impl<'a> AnalyticsDropSite<'a> { } } + fn review(input: &'a PendingReviewState) -> Self { + Self { + event_name: "review", + thread_id: &input.thread_id, + turn_id: Some(&input.turn_id), + review_id: Some(&input.review_id), + item_id: input.item_id.as_deref(), + } + } + fn compaction(input: &'a CodexCompactionEvent) -> Self { Self { event_name: "compaction", @@ -195,6 +224,30 @@ enum MissingAnalyticsContext { ThreadMetadata, } +#[derive(Clone)] +struct PendingReviewState { + thread_id: String, + turn_id: String, + item_id: Option, + review_id: String, + subject_kind: ReviewSubjectKind, + subject_name: String, + trigger: ReviewTrigger, + started_at_ms: u64, + requested_additional_permissions: bool, + requested_network_access: bool, +} + +#[derive(Clone, Default)] +struct ItemReviewSummary { + review_count: u64, + guardian_review_count: u64, + user_review_count: u64, + final_approval_outcome: Option, + requested_additional_permissions: bool, + requested_network_access: bool, +} + #[derive(Clone)] struct ThreadMetadataState { thread_source: Option, @@ -320,13 +373,23 @@ impl AnalyticsReducer { self.ingest_notification(*notification, out); } AnalyticsFact::ServerRequest { - connection_id: _connection_id, - request: _request, - } => {} + connection_id, + request, + } => { + self.ingest_server_request(connection_id, *request); + } AnalyticsFact::ServerResponse { - response: _response, - .. - } => {} + completed_at_ms, + response, + } => { + self.ingest_server_response(completed_at_ms, *response, out); + } + AnalyticsFact::ServerRequestAborted { + completed_at_ms, + request_id, + } => { + self.ingest_server_request_aborted(completed_at_ms, request_id, out); + } AnalyticsFact::Custom(input) => match input { CustomAnalyticsFact::SubAgentThreadStarted(input) => { self.ingest_subagent_thread_started(input, out); @@ -691,6 +754,216 @@ impl AnalyticsReducer { } } + fn ingest_server_request(&mut self, _connection_id: u64, request: ServerRequest) { + match request { + ServerRequest::CommandExecutionRequestApproval { request_id, params } => { + let is_network_access_review = params.network_approval_context.is_some(); + let requested_network_access = is_network_access_review + || params + .proposed_network_policy_amendments + .as_ref() + .is_some_and(|amendments| !amendments.is_empty()) + || params + .additional_permissions + .as_ref() + .and_then(|permissions| permissions.network.as_ref()) + .and_then(|network| network.enabled) + .unwrap_or(false); + let requested_additional_permissions = params.additional_permissions.is_some(); + let trigger = if params.approval_id.is_some() { + ReviewTrigger::ExecveIntercept + } else if requested_network_access { + ReviewTrigger::NetworkPolicyDenial + } else if requested_additional_permissions { + ReviewTrigger::SandboxDenial + } else { + ReviewTrigger::Initial + }; + let Some(started_at_ms) = option_i64_to_u64(Some(params.started_at_ms)) else { + return; + }; + self.pending_reviews.insert( + request_id.clone(), + PendingReviewState { + thread_id: params.thread_id, + turn_id: params.turn_id, + item_id: Some(params.item_id), + review_id: user_review_id(&request_id), + subject_kind: if is_network_access_review { + ReviewSubjectKind::NetworkAccess + } else { + ReviewSubjectKind::CommandExecution + }, + subject_name: if is_network_access_review { + "network_access".to_string() + } else { + app_server_guardian_command_tool_name(params.source).to_string() + }, + trigger, + started_at_ms, + requested_additional_permissions, + requested_network_access, + }, + ); + } + ServerRequest::FileChangeRequestApproval { request_id, params } => { + let requested_additional_permissions = params.grant_root.is_some(); + let Some(started_at_ms) = option_i64_to_u64(Some(params.started_at_ms)) else { + return; + }; + self.pending_reviews.insert( + request_id.clone(), + PendingReviewState { + thread_id: params.thread_id, + turn_id: params.turn_id, + item_id: Some(params.item_id), + review_id: user_review_id(&request_id), + subject_kind: ReviewSubjectKind::FileChange, + subject_name: "apply_patch".to_string(), + trigger: if requested_additional_permissions { + ReviewTrigger::SandboxDenial + } else { + ReviewTrigger::Initial + }, + started_at_ms, + requested_additional_permissions, + requested_network_access: false, + }, + ); + } + ServerRequest::PermissionsRequestApproval { request_id, params } => { + let requested_network_access = params + .permissions + .network + .as_ref() + .and_then(|network| network.enabled) + .unwrap_or(false); + let requested_additional_permissions = + requested_network_access || params.permissions.file_system.is_some(); + let trigger = if requested_network_access { + ReviewTrigger::NetworkPolicyDenial + } else if requested_additional_permissions { + ReviewTrigger::SandboxDenial + } else { + ReviewTrigger::Initial + }; + let Some(started_at_ms) = option_i64_to_u64(Some(params.started_at_ms)) else { + return; + }; + self.pending_reviews.insert( + request_id.clone(), + PendingReviewState { + thread_id: params.thread_id, + turn_id: params.turn_id, + item_id: Some(params.item_id), + review_id: user_review_id(&request_id), + subject_kind: ReviewSubjectKind::Permissions, + subject_name: "permissions".to_string(), + trigger, + started_at_ms, + requested_additional_permissions, + requested_network_access, + }, + ); + } + _ => {} + } + } + + fn ingest_server_response( + &mut self, + completed_at_ms: u64, + response: ServerResponse, + out: &mut Vec, + ) { + match response { + ServerResponse::CommandExecutionRequestApproval { + request_id, + response, + } => { + let Some(pending_review) = self.pending_reviews.remove(&request_id) else { + return; + }; + let (status, resolution) = command_execution_review_result(response.decision); + self.emit_review_event( + pending_review, + Reviewer::User, + status, + resolution, + completed_at_ms, + out, + ); + } + ServerResponse::FileChangeRequestApproval { + request_id, + response, + } => { + let Some(pending_review) = self.pending_reviews.remove(&request_id) else { + return; + }; + let (status, resolution) = file_change_review_result(response.decision); + self.emit_review_event( + pending_review, + Reviewer::User, + status, + resolution, + completed_at_ms, + out, + ); + } + ServerResponse::PermissionsRequestApproval { + request_id, + response, + } => { + let Some(pending_review) = self.pending_reviews.remove(&request_id) else { + return; + }; + let (status, resolution) = if response.permissions.network.is_none() + && response.permissions.file_system.is_none() + { + (ReviewStatus::Denied, ReviewResolution::None) + } else { + match response.scope { + PermissionGrantScope::Turn => { + (ReviewStatus::Approved, ReviewResolution::None) + } + PermissionGrantScope::Session => { + (ReviewStatus::Approved, ReviewResolution::SessionApproval) + } + } + }; + self.emit_review_event( + pending_review, + Reviewer::User, + status, + resolution, + completed_at_ms, + out, + ); + } + _ => {} + } + } + + fn ingest_server_request_aborted( + &mut self, + completed_at_ms: u64, + request_id: RequestId, + out: &mut Vec, + ) { + let Some(pending_review) = self.pending_reviews.remove(&request_id) else { + return; + }; + self.emit_review_event( + pending_review, + Reviewer::User, + ReviewStatus::Aborted, + ReviewResolution::None, + completed_at_ms, + out, + ); + } + fn ingest_error_response( &mut self, connection_id: u64, @@ -791,17 +1064,25 @@ impl AnalyticsReducer { else { return; }; - if let Some(event) = tool_item_event( - ¬ification.thread_id, - ¬ification.turn_id, - ¬ification.item, + if let Some(event) = tool_item_event(ToolItemEventInput { + thread_id: ¬ification.thread_id, + turn_id: ¬ification.turn_id, + item: ¬ification.item, started_at_ms, completed_at_ms, connection_state, thread_metadata, - ) { + review_summary: self.item_review_summaries.get(&key), + }) { out.push(event); } + self.item_review_summaries.remove(&key); + } + ServerNotification::ItemGuardianApprovalReviewStarted(notification) => { + let _ = notification; + } + ServerNotification::ItemGuardianApprovalReviewCompleted(notification) => { + self.ingest_guardian_review_completed(notification, out); } ServerNotification::TurnStarted(notification) => { let turn_state = self.turns.entry(notification.turn.id).or_insert(TurnState { @@ -921,6 +1202,48 @@ impl AnalyticsReducer { ))); } + fn ingest_guardian_review_completed( + &mut self, + notification: codex_app_server_protocol::ItemGuardianApprovalReviewCompletedNotification, + out: &mut Vec, + ) { + let Some((status, resolution)) = guardian_review_result(notification.review.status) else { + return; + }; + let (subject_kind, subject_name, trigger) = + guardian_review_subject_metadata(¬ification.action); + let Some(started_at_ms) = option_i64_to_u64(Some(notification.started_at_ms)) else { + return; + }; + let pending_review = PendingReviewState { + thread_id: notification.thread_id, + turn_id: notification.turn_id, + item_id: notification.target_item_id, + review_id: notification.review_id, + subject_kind, + subject_name, + trigger, + started_at_ms, + requested_additional_permissions: guardian_review_requested_additional_permissions( + ¬ification.action, + ), + requested_network_access: guardian_review_requested_network_access( + ¬ification.action, + ), + }; + let Some(completed_at_ms) = option_i64_to_u64(Some(notification.completed_at_ms)) else { + return; + }; + self.emit_review_event( + pending_review, + Reviewer::Guardian, + status, + resolution, + completed_at_ms, + out, + ); + } + fn ingest_turn_steer_response( &mut self, connection_id: u64, @@ -986,6 +1309,73 @@ impl AnalyticsReducer { })); } + fn emit_review_event( + &mut self, + pending_review: PendingReviewState, + reviewer: Reviewer, + status: ReviewStatus, + resolution: ReviewResolution, + completed_at_ms: u64, + out: &mut Vec, + ) { + if let Some(item_key) = item_review_summary_key(&pending_review) { + self.record_item_review_summary( + item_key, + reviewer, + status, + resolution, + &pending_review, + ); + } + let Some((connection_state, thread_metadata)) = + self.thread_context_or_warn(AnalyticsDropSite::review(&pending_review)) + else { + return; + }; + out.push(TrackEventRequest::ReviewEvent(CodexReviewEventRequest { + event_type: "codex_review_event", + event_params: CodexReviewEventParams { + thread_id: pending_review.thread_id, + turn_id: pending_review.turn_id, + item_id: pending_review.item_id, + review_id: pending_review.review_id, + app_server_client: connection_state.app_server_client.clone(), + runtime: connection_state.runtime.clone(), + thread_source: thread_metadata.thread_source, + subagent_source: thread_metadata.subagent_source.clone(), + parent_thread_id: thread_metadata.parent_thread_id.clone(), + subject_kind: pending_review.subject_kind, + subject_name: pending_review.subject_name, + reviewer, + trigger: pending_review.trigger, + status, + resolution, + started_at_ms: pending_review.started_at_ms, + completed_at_ms, + duration_ms: observed_duration_ms(pending_review.started_at_ms, completed_at_ms), + }, + })); + } + + fn record_item_review_summary( + &mut self, + item_key: ToolItemKey, + reviewer: Reviewer, + status: ReviewStatus, + resolution: ReviewResolution, + pending_review: &PendingReviewState, + ) { + let summary = self.item_review_summaries.entry(item_key).or_default(); + summary.review_count += 1; + match reviewer { + Reviewer::Guardian => summary.guardian_review_count += 1, + Reviewer::User => summary.user_review_count += 1, + } + summary.final_approval_outcome = Some(final_approval_outcome(reviewer, status, resolution)); + summary.requested_additional_permissions |= pending_review.requested_additional_permissions; + summary.requested_network_access |= pending_review.requested_network_access; + } + fn maybe_emit_turn_event(&mut self, turn_id: &str, out: &mut Vec) { let Some(turn_state) = self.turns.get(turn_id) else { return; @@ -1117,21 +1507,41 @@ fn tracked_tool_item_id(item: &ThreadItem) -> Option<&str> { } } -fn tool_item_event( - thread_id: &str, - turn_id: &str, - item: &ThreadItem, +fn item_review_summary_key(pending_review: &PendingReviewState) -> Option { + match pending_review.subject_kind { + ReviewSubjectKind::CommandExecution + | ReviewSubjectKind::FileChange + | ReviewSubjectKind::McpToolCall => Some(ToolItemKey { + thread_id: pending_review.thread_id.clone(), + turn_id: pending_review.turn_id.clone(), + item_id: pending_review.item_id.clone()?, + }), + ReviewSubjectKind::Permissions | ReviewSubjectKind::NetworkAccess => None, + } +} + +struct ToolItemEventInput<'a> { + thread_id: &'a str, + turn_id: &'a str, + item: &'a ThreadItem, started_at_ms: u64, completed_at_ms: u64, - connection_state: &ConnectionState, - thread_metadata: &ThreadMetadataState, -) -> Option { - let context = ToolItemContext { + connection_state: &'a ConnectionState, + thread_metadata: &'a ThreadMetadataState, + review_summary: Option<&'a ItemReviewSummary>, +} + +fn tool_item_event(input: ToolItemEventInput<'_>) -> Option { + let ToolItemEventInput { + thread_id, + turn_id, + item, started_at_ms, completed_at_ms, connection_state, thread_metadata, - }; + review_summary, + } = input; match item { ThreadItem::CommandExecution { id, @@ -1154,7 +1564,13 @@ fn tool_item_event( failure_kind, execution_duration_ms: option_i64_to_u64(*duration_ms), }, - context, + ToolItemContext { + started_at_ms, + completed_at_ms, + connection_state, + thread_metadata, + review_summary, + }, ); Some(TrackEventRequest::CommandExecution( CodexCommandExecutionEventRequest { @@ -1189,7 +1605,13 @@ fn tool_item_event( failure_kind, execution_duration_ms: None, }, - context, + ToolItemContext { + started_at_ms, + completed_at_ms, + connection_state, + thread_metadata, + review_summary, + }, ); Some(TrackEventRequest::FileChange(CodexFileChangeEventRequest { event_type: "codex_file_change_event", @@ -1223,7 +1645,13 @@ fn tool_item_event( failure_kind, execution_duration_ms: option_i64_to_u64(*duration_ms), }, - context, + ToolItemContext { + started_at_ms, + completed_at_ms, + connection_state, + thread_metadata, + review_summary, + }, ); Some(TrackEventRequest::McpToolCall( CodexMcpToolCallEventRequest { @@ -1260,7 +1688,13 @@ fn tool_item_event( failure_kind, execution_duration_ms: option_i64_to_u64(*duration_ms), }, - context, + ToolItemContext { + started_at_ms, + completed_at_ms, + connection_state, + thread_metadata, + review_summary, + }, ); Some(TrackEventRequest::DynamicToolCall( CodexDynamicToolCallEventRequest { @@ -1298,7 +1732,13 @@ fn tool_item_event( failure_kind, execution_duration_ms: None, }, - context, + ToolItemContext { + started_at_ms, + completed_at_ms, + connection_state, + thread_metadata, + review_summary, + }, ); Some(TrackEventRequest::CollabAgentToolCall( CodexCollabAgentToolCallEventRequest { @@ -1347,7 +1787,13 @@ fn tool_item_event( failure_kind: None, execution_duration_ms: None, }, - context, + ToolItemContext { + started_at_ms, + completed_at_ms, + connection_state, + thread_metadata, + review_summary, + }, ); Some(TrackEventRequest::WebSearch(CodexWebSearchEventRequest { event_type: "codex_web_search_event", @@ -1377,7 +1823,13 @@ fn tool_item_event( failure_kind, execution_duration_ms: None, }, - context, + ToolItemContext { + started_at_ms, + completed_at_ms, + connection_state, + thread_metadata, + review_summary, + }, ); Some(TrackEventRequest::ImageGeneration( CodexImageGenerationEventRequest { @@ -1431,6 +1883,7 @@ struct ToolItemContext<'a> { completed_at_ms: u64, connection_state: &'a ConnectionState, thread_metadata: &'a ThreadMetadataState, + review_summary: Option<&'a ItemReviewSummary>, } fn tool_item_base( @@ -1442,6 +1895,7 @@ fn tool_item_base( context: ToolItemContext<'_>, ) -> CodexToolItemEventBase { let thread_metadata = context.thread_metadata; + let review_summary = context.review_summary.cloned().unwrap_or_default(); CodexToolItemEventBase { thread_id: thread_id.to_string(), turn_id: turn_id.to_string(), @@ -1459,14 +1913,16 @@ fn tool_item_base( // full upstream execution time. duration_ms: observed_duration_ms(context.started_at_ms, context.completed_at_ms), execution_duration_ms: outcome.execution_duration_ms, - review_count: 0, - guardian_review_count: 0, - user_review_count: 0, - final_approval_outcome: ToolItemFinalApprovalOutcome::Unknown, + review_count: review_summary.review_count, + guardian_review_count: review_summary.guardian_review_count, + user_review_count: review_summary.user_review_count, + final_approval_outcome: review_summary + .final_approval_outcome + .unwrap_or(FinalApprovalOutcome::Unknown), terminal_status: outcome.terminal_status, failure_kind: outcome.failure_kind, - requested_additional_permissions: false, - requested_network_access: false, + requested_additional_permissions: review_summary.requested_additional_permissions, + requested_network_access: review_summary.requested_network_access, } } @@ -1474,6 +1930,187 @@ fn observed_duration_ms(started_at_ms: u64, completed_at_ms: u64) -> Option completed_at_ms.checked_sub(started_at_ms) } +fn user_review_id(request_id: &RequestId) -> String { + format!("user:{request_id}") +} + +fn command_execution_review_result( + decision: CommandExecutionApprovalDecision, +) -> (ReviewStatus, ReviewResolution) { + match decision { + CommandExecutionApprovalDecision::Accept => { + (ReviewStatus::Approved, ReviewResolution::None) + } + CommandExecutionApprovalDecision::AcceptForSession => { + (ReviewStatus::Approved, ReviewResolution::SessionApproval) + } + CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { .. } => ( + ReviewStatus::Approved, + ReviewResolution::ExecPolicyAmendment, + ), + CommandExecutionApprovalDecision::ApplyNetworkPolicyAmendment { + network_policy_amendment, + } => match network_policy_amendment.action { + NetworkPolicyRuleAction::Allow => ( + ReviewStatus::Approved, + ReviewResolution::NetworkPolicyAmendment, + ), + NetworkPolicyRuleAction::Deny => ( + ReviewStatus::Denied, + ReviewResolution::NetworkPolicyAmendment, + ), + }, + CommandExecutionApprovalDecision::Decline => (ReviewStatus::Denied, ReviewResolution::None), + CommandExecutionApprovalDecision::Cancel => (ReviewStatus::Aborted, ReviewResolution::None), + } +} + +fn file_change_review_result( + decision: FileChangeApprovalDecision, +) -> (ReviewStatus, ReviewResolution) { + match decision { + FileChangeApprovalDecision::Accept => (ReviewStatus::Approved, ReviewResolution::None), + FileChangeApprovalDecision::AcceptForSession => { + (ReviewStatus::Approved, ReviewResolution::SessionApproval) + } + FileChangeApprovalDecision::Decline => (ReviewStatus::Denied, ReviewResolution::None), + FileChangeApprovalDecision::Cancel => (ReviewStatus::Aborted, ReviewResolution::None), + } +} + +fn guardian_review_result( + status: GuardianApprovalReviewStatus, +) -> Option<(ReviewStatus, ReviewResolution)> { + match status { + GuardianApprovalReviewStatus::InProgress => None, + GuardianApprovalReviewStatus::Approved => { + Some((ReviewStatus::Approved, ReviewResolution::None)) + } + GuardianApprovalReviewStatus::Denied => { + Some((ReviewStatus::Denied, ReviewResolution::None)) + } + GuardianApprovalReviewStatus::TimedOut => { + Some((ReviewStatus::TimedOut, ReviewResolution::None)) + } + GuardianApprovalReviewStatus::Aborted => { + Some((ReviewStatus::Aborted, ReviewResolution::None)) + } + } +} + +fn guardian_review_subject_metadata( + action: &GuardianApprovalReviewAction, +) -> (ReviewSubjectKind, String, ReviewTrigger) { + match action { + GuardianApprovalReviewAction::Command { source, .. } => ( + ReviewSubjectKind::CommandExecution, + app_server_guardian_command_tool_name(*source).to_string(), + ReviewTrigger::Initial, + ), + GuardianApprovalReviewAction::Execve { source, .. } => ( + ReviewSubjectKind::CommandExecution, + app_server_guardian_command_tool_name(*source).to_string(), + ReviewTrigger::ExecveIntercept, + ), + GuardianApprovalReviewAction::ApplyPatch { .. } => ( + ReviewSubjectKind::FileChange, + "apply_patch".to_string(), + ReviewTrigger::SandboxDenial, + ), + GuardianApprovalReviewAction::NetworkAccess { .. } => ( + ReviewSubjectKind::NetworkAccess, + "network_access".to_string(), + ReviewTrigger::NetworkPolicyDenial, + ), + GuardianApprovalReviewAction::RequestPermissions { permissions, .. } => { + let requested_network_access = permissions + .network + .as_ref() + .and_then(|network| network.enabled) + .unwrap_or(false); + let trigger = if requested_network_access { + ReviewTrigger::NetworkPolicyDenial + } else if permissions.file_system.is_some() { + ReviewTrigger::SandboxDenial + } else { + ReviewTrigger::Initial + }; + ( + ReviewSubjectKind::Permissions, + "permissions".to_string(), + trigger, + ) + } + GuardianApprovalReviewAction::McpToolCall { tool_name, .. } => ( + ReviewSubjectKind::McpToolCall, + tool_name.clone(), + ReviewTrigger::Initial, + ), + } +} + +fn guardian_review_requested_additional_permissions(action: &GuardianApprovalReviewAction) -> bool { + match action { + GuardianApprovalReviewAction::ApplyPatch { .. } + | GuardianApprovalReviewAction::NetworkAccess { .. } => true, + GuardianApprovalReviewAction::RequestPermissions { permissions, .. } => { + guardian_review_request_permissions_network_enabled(permissions) + || permissions.file_system.is_some() + } + GuardianApprovalReviewAction::Command { .. } + | GuardianApprovalReviewAction::Execve { .. } + | GuardianApprovalReviewAction::McpToolCall { .. } => false, + } +} + +fn guardian_review_requested_network_access(action: &GuardianApprovalReviewAction) -> bool { + match action { + GuardianApprovalReviewAction::NetworkAccess { .. } => true, + GuardianApprovalReviewAction::RequestPermissions { permissions, .. } => { + guardian_review_request_permissions_network_enabled(permissions) + } + GuardianApprovalReviewAction::ApplyPatch { .. } + | GuardianApprovalReviewAction::Command { .. } + | GuardianApprovalReviewAction::Execve { .. } + | GuardianApprovalReviewAction::McpToolCall { .. } => false, + } +} + +fn guardian_review_request_permissions_network_enabled( + permissions: &RequestPermissionProfile, +) -> bool { + permissions + .network + .as_ref() + .and_then(|network| network.enabled) + .unwrap_or(false) +} + +fn app_server_guardian_command_tool_name(source: AppServerGuardianCommandSource) -> &'static str { + match source { + AppServerGuardianCommandSource::Shell => "shell", + AppServerGuardianCommandSource::UnifiedExec => "unified_exec", + } +} + +fn final_approval_outcome( + reviewer: Reviewer, + status: ReviewStatus, + resolution: ReviewResolution, +) -> FinalApprovalOutcome { + match (reviewer, status, resolution) { + (Reviewer::Guardian, ReviewStatus::Approved, _) => FinalApprovalOutcome::GuardianApproved, + (Reviewer::Guardian, ReviewStatus::Denied, _) => FinalApprovalOutcome::GuardianDenied, + (Reviewer::Guardian, _, _) => FinalApprovalOutcome::GuardianAborted, + (Reviewer::User, ReviewStatus::Approved, ReviewResolution::SessionApproval) => { + FinalApprovalOutcome::UserApprovedForSession + } + (Reviewer::User, ReviewStatus::Approved, _) => FinalApprovalOutcome::UserApproved, + (Reviewer::User, ReviewStatus::Denied, _) => FinalApprovalOutcome::UserDenied, + (Reviewer::User, _, _) => FinalApprovalOutcome::UserAborted, + } +} + fn command_execution_tool_name(source: CommandExecutionSource) -> &'static str { match source { CommandExecutionSource::UnifiedExecStartup @@ -1873,4 +2510,25 @@ mod tests { "external_sandbox" ); } + + #[test] + fn app_server_guardian_command_tool_name_maps_all_sources() { + assert_eq!( + app_server_guardian_command_tool_name(AppServerGuardianCommandSource::Shell), + "shell" + ); + assert_eq!( + app_server_guardian_command_tool_name(AppServerGuardianCommandSource::UnifiedExec), + "unified_exec" + ); + } + + #[test] + fn guardian_review_result_maps_terminal_statuses() { + assert!(guardian_review_result(GuardianApprovalReviewStatus::InProgress).is_none()); + assert!(matches!( + guardian_review_result(GuardianApprovalReviewStatus::TimedOut), + Some((ReviewStatus::TimedOut, ReviewResolution::None)) + )); + } } diff --git a/codex-rs/app-server-protocol/schema/json/ClientRequest.json b/codex-rs/app-server-protocol/schema/json/ClientRequest.json index c60acbdf30aa..bcb522ce2247 100644 --- a/codex-rs/app-server-protocol/schema/json/ClientRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ClientRequest.json @@ -6206,4 +6206,4 @@ } ], "title": "ClientRequest" -} +} \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json index 5b6c4cd18534..33a43691a5ef 100644 --- a/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json +++ b/codex-rs/app-server-protocol/schema/json/CommandExecutionRequestApprovalParams.json @@ -469,6 +469,13 @@ } ] }, + "GuardianCommandSource": { + "enum": [ + "shell", + "unifiedExec" + ], + "type": "string" + }, "NetworkApprovalContext": { "properties": { "host": { @@ -593,6 +600,15 @@ "null" ] }, + "source": { + "allOf": [ + { + "$ref": "#/definitions/GuardianCommandSource" + } + ], + "default": "shell", + "description": "Originating command tool for this approval request.\n\nUses `#[serde(default)]` for backwards compatibility with older senders." + }, "startedAtMs": { "description": "Unix timestamp (in milliseconds) when this approval request started.", "format": "int64", diff --git a/codex-rs/app-server-protocol/schema/json/ServerRequest.json b/codex-rs/app-server-protocol/schema/json/ServerRequest.json index 9844eac0b835..3362ae6527d3 100644 --- a/codex-rs/app-server-protocol/schema/json/ServerRequest.json +++ b/codex-rs/app-server-protocol/schema/json/ServerRequest.json @@ -417,6 +417,15 @@ "null" ] }, + "source": { + "allOf": [ + { + "$ref": "#/definitions/GuardianCommandSource" + } + ], + "default": "shell", + "description": "Originating command tool for this approval request.\n\nUses `#[serde(default)]` for backwards compatibility with older senders." + }, "startedAtMs": { "description": "Unix timestamp (in milliseconds) when this approval request started.", "format": "int64", @@ -820,6 +829,13 @@ } ] }, + "GuardianCommandSource": { + "enum": [ + "shell", + "unifiedExec" + ], + "type": "string" + }, "McpElicitationArrayType": { "enum": [ "array" diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index d11d4998458d..16ce659b49b0 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -2146,6 +2146,15 @@ "null" ] }, + "source": { + "allOf": [ + { + "$ref": "#/definitions/v2/GuardianCommandSource" + } + ], + "default": "shell", + "description": "Originating command tool for this approval request.\n\nUses `#[serde(default)]` for backwards compatibility with older senders." + }, "startedAtMs": { "description": "Unix timestamp (in milliseconds) when this approval request started.", "format": "int64", @@ -18425,4 +18434,4 @@ }, "title": "CodexAppServerProtocol", "type": "object" -} +} \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json index 41168a07322f..7d8ac9ddf1ca 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.v2.schemas.json @@ -16292,4 +16292,4 @@ }, "title": "CodexAppServerProtocolV2", "type": "object" -} +} \ No newline at end of file diff --git a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts index 0e9100836a61..841d4cbe5693 100644 --- a/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts +++ b/codex-rs/app-server-protocol/schema/typescript/v2/CommandExecutionRequestApprovalParams.ts @@ -4,6 +4,7 @@ import type { AbsolutePathBuf } from "../AbsolutePathBuf"; import type { CommandAction } from "./CommandAction"; import type { ExecPolicyAmendment } from "./ExecPolicyAmendment"; +import type { GuardianCommandSource } from "./GuardianCommandSource"; import type { NetworkApprovalContext } from "./NetworkApprovalContext"; import type { NetworkPolicyAmendment } from "./NetworkPolicyAmendment"; @@ -20,6 +21,11 @@ startedAtMs: number, /** * (a UUID) used to disambiguate routing. */ approvalId?: string | null, /** + * Originating command tool for this approval request. + * + * Uses `#[serde(default)]` for backwards compatibility with older senders. + */ +source: GuardianCommandSource, /** * Optional explanatory reason (e.g. request for network access). */ reason?: string | null, /** diff --git a/codex-rs/app-server-protocol/src/protocol/common.rs b/codex-rs/app-server-protocol/src/protocol/common.rs index bf0b3c87b149..cd4aa0d1a8bb 100644 --- a/codex-rs/app-server-protocol/src/protocol/common.rs +++ b/codex-rs/app-server-protocol/src/protocol/common.rs @@ -2969,6 +2969,7 @@ mod tests { item_id: "call_123".to_string(), started_at_ms: 0, approval_id: None, + source: v2::GuardianCommandSource::Shell, reason: None, network_approval_context: None, command: Some("cat file".to_string()), diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 0e22c485900e..4813d3877dfb 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -486,6 +486,10 @@ pub enum GuardianCommandSource { UnifiedExec, } +fn default_guardian_command_source() -> GuardianCommandSource { + GuardianCommandSource::Shell +} + impl From for GuardianCommandSource { fn from(value: CoreGuardianCommandSource) -> Self { match value { @@ -1270,6 +1274,11 @@ pub struct CommandExecutionRequestApprovalParams { #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] pub approval_id: Option, + /// Originating command tool for this approval request. + /// + /// Uses `#[serde(default)]` for backwards compatibility with older senders. + #[serde(default = "default_guardian_command_source")] + pub source: GuardianCommandSource, /// Optional explanatory reason (e.g. request for network access). #[serde(default, skip_serializing_if = "Option::is_none")] #[ts(optional = nullable)] diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index e67f6e02f3bf..6a4811f1d0e5 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -1947,6 +1947,7 @@ impl CodexClient { item_id, started_at_ms: _, approval_id, + source: _, reason, network_approval_context, command, diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs index 1f2f289b05bd..a1234e27c737 100644 --- a/codex-rs/app-server/src/bespoke_event_handling.rs +++ b/codex-rs/app-server/src/bespoke_event_handling.rs @@ -544,6 +544,7 @@ pub(crate) async fn apply_bespoke_event_handling( approval_id, turn_id, started_at_ms, + source, command, cwd, reason, @@ -619,6 +620,7 @@ pub(crate) async fn apply_bespoke_event_handling( item_id: call_id.clone(), started_at_ms, approval_id: approval_id.clone(), + source: source.into(), reason, network_approval_context, command, diff --git a/codex-rs/app-server/src/outgoing_message.rs b/codex-rs/app-server/src/outgoing_message.rs index cbe196cd9869..39cf1b45720d 100644 --- a/codex-rs/app-server/src/outgoing_message.rs +++ b/codex-rs/app-server/src/outgoing_message.rs @@ -380,6 +380,8 @@ impl OutgoingMessageSender { match entry { Some((id, entry)) => { warn!("client responded with error for {id:?}: {error:?}"); + self.analytics_events_client + .track_server_request_aborted(now_unix_timestamp_ms(), id.clone()); if let Err(err) = entry.callback.send(Err(error)) { warn!("could not notify callback for {id:?} due to: {err:?}"); } @@ -391,7 +393,14 @@ impl OutgoingMessageSender { } pub(crate) async fn cancel_request(&self, id: &RequestId) -> bool { - self.take_request_callback(id).await.is_some() + let entry = self.take_request_callback(id).await; + if let Some((request_id, _entry)) = entry { + self.analytics_events_client + .track_server_request_aborted(now_unix_timestamp_ms(), request_id); + true + } else { + false + } } pub(crate) async fn cancel_all_requests(&self, error: Option) { @@ -403,12 +412,14 @@ impl OutgoingMessageSender { .collect::>() }; - if let Some(error) = error { - for entry in entries { - if let Err(err) = entry.callback.send(Err(error.clone())) { - let request_id = entry.request.id(); - warn!("could not notify callback for {request_id:?} due to: {err:?}"); - } + for entry in entries { + self.analytics_events_client + .track_server_request_aborted(now_unix_timestamp_ms(), entry.request.id().clone()); + if let Some(error) = error.as_ref() + && let Err(err) = entry.callback.send(Err(error.clone())) + { + let request_id = entry.request.id(); + warn!("could not notify callback for {request_id:?} due to: {err:?}"); } } } @@ -459,12 +470,14 @@ impl OutgoingMessageSender { entries }; - if let Some(error) = error { - for entry in entries { - if let Err(err) = entry.callback.send(Err(error.clone())) { - let request_id = entry.request.id(); - warn!("could not notify callback for {request_id:?} due to: {err:?}",); - } + for entry in entries { + self.analytics_events_client + .track_server_request_aborted(now_unix_timestamp_ms(), entry.request.id().clone()); + if let Some(error) = error.as_ref() + && let Err(err) = entry.callback.send(Err(error.clone())) + { + let request_id = entry.request.id(); + warn!("could not notify callback for {request_id:?} due to: {err:?}",); } } } @@ -918,6 +931,7 @@ mod tests { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: None, + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: None, network_approval_context: None, command: Some("echo hi".to_string()), diff --git a/codex-rs/app-server/src/transport_tests.rs b/codex-rs/app-server/src/transport_tests.rs index 5790e46a1746..181bba00a7af 100644 --- a/codex-rs/app-server/src/transport_tests.rs +++ b/codex-rs/app-server/src/transport_tests.rs @@ -260,6 +260,7 @@ async fn command_execution_request_approval_strips_additional_permissions_withou item_id: "call_123".to_string(), started_at_ms: 0, approval_id: None, + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: Some("Need extra read access".to_string()), network_approval_context: None, command: Some("cat file".to_string()), @@ -325,6 +326,7 @@ async fn command_execution_request_approval_keeps_additional_permissions_with_ca item_id: "call_123".to_string(), started_at_ms: 0, approval_id: None, + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: Some("Need extra read access".to_string()), network_approval_context: None, command: Some("cat file".to_string()), diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index a89d8fc9737c..afca6351dc9e 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -439,6 +439,7 @@ async fn handle_exec_approval( let ExecApprovalRequestEvent { call_id, approval_id, + source, command, cwd, reason, @@ -484,6 +485,7 @@ async fn handle_exec_approval( parent_ctx, call_id, approval_id, + source, command, cwd, reason, diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index ecd392e3e76e..438d9601cfb4 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -315,6 +315,7 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f approval_id: Some("callback-approval-1".to_string()), turn_id: "child-turn-1".to_string(), started_at_ms: 0, + source: GuardianCommandSource::Shell, command: vec!["rm".to_string(), "-rf".to_string(), "tmp".to_string()], cwd: test_path_buf("/tmp").abs(), reason: Some("unsafe subcommand".to_string()), diff --git a/codex-rs/core/src/guardian/review.rs b/codex-rs/core/src/guardian/review.rs index bba2167cefee..5f0aa67b16a4 100644 --- a/codex-rs/core/src/guardian/review.rs +++ b/codex-rs/core/src/guardian/review.rs @@ -1,5 +1,3 @@ -use std::sync::Arc; - use codex_analytics::GuardianApprovalRequestSource; use codex_analytics::GuardianReviewAnalyticsResult; use codex_analytics::GuardianReviewDecision; @@ -18,6 +16,7 @@ use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::TurnAbortReason; use codex_protocol::protocol::WarningEvent; +use std::sync::Arc; use tokio::sync::oneshot; use tokio_util::sync::CancellationToken; @@ -163,11 +162,12 @@ fn track_guardian_review( session: &Session, tracking: &GuardianReviewTrackContext, result: GuardianReviewAnalyticsResult, + completed_at_ms: u64, ) { session .services .analytics_events_client - .track_guardian_review(tracking, result); + .track_guardian_review(tracking, result, completed_at_ms); } async fn record_guardian_non_denial(session: &Arc, turn_id: &str) { @@ -276,6 +276,7 @@ async fn run_guardian_review( .as_ref() .is_some_and(CancellationToken::is_cancelled) { + let completed_at_ms = now_unix_timestamp_ms(); track_guardian_review( session.as_ref(), &review_tracking, @@ -285,6 +286,7 @@ async fn run_guardian_review( failure_reason: Some(GuardianReviewFailureReason::Cancelled), ..GuardianReviewAnalyticsResult::without_session() }, + completed_at_ms.try_into().unwrap_or_default(), ); session .send_event( @@ -294,7 +296,7 @@ async fn run_guardian_review( target_item_id, turn_id: assessment_turn_id.clone(), started_at_ms, - completed_at_ms: Some(now_unix_timestamp_ms()), + completed_at_ms: Some(completed_at_ms), status: GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, @@ -320,9 +322,10 @@ async fn run_guardian_review( )) .await; - let (assessment, count_denial_for_circuit_breaker) = match outcome { + let (assessment, count_denial_for_circuit_breaker, completed_at_ms) = match outcome { GuardianReviewOutcome::Completed(assessment) => { let approved = matches!(assessment.outcome, GuardianAssessmentOutcome::Allow); + let completed_at_ms = now_unix_timestamp_ms(); track_guardian_review( session.as_ref(), &review_tracking, @@ -343,16 +346,22 @@ async fn run_guardian_review( outcome: Some(assessment.outcome), ..analytics_result }, + completed_at_ms.try_into().unwrap_or_default(), ); let count_denial_for_circuit_breaker = matches!(assessment.outcome, GuardianAssessmentOutcome::Deny); - (assessment, count_denial_for_circuit_breaker) + ( + assessment, + count_denial_for_circuit_breaker, + completed_at_ms, + ) } GuardianReviewOutcome::Error(error) => match error { GuardianReviewError::Timeout => { let rationale = "Automatic approval review timed out while evaluating the requested approval." .to_string(); + let completed_at_ms = now_unix_timestamp_ms(); track_guardian_review( session.as_ref(), &review_tracking, @@ -362,6 +371,7 @@ async fn run_guardian_review( failure_reason: Some(error.failure_reason()), ..analytics_result }, + completed_at_ms.try_into().unwrap_or_default(), ); session .send_event( @@ -379,7 +389,7 @@ async fn run_guardian_review( target_item_id, turn_id: assessment_turn_id.clone(), started_at_ms, - completed_at_ms: Some(now_unix_timestamp_ms()), + completed_at_ms: Some(completed_at_ms), status: GuardianAssessmentStatus::TimedOut, risk_level: None, user_authorization: None, @@ -393,6 +403,7 @@ async fn run_guardian_review( return ReviewDecision::TimedOut; } GuardianReviewError::Cancelled => { + let completed_at_ms = now_unix_timestamp_ms(); track_guardian_review( session.as_ref(), &review_tracking, @@ -402,6 +413,7 @@ async fn run_guardian_review( failure_reason: Some(error.failure_reason()), ..analytics_result }, + completed_at_ms.try_into().unwrap_or_default(), ); session .send_event( @@ -411,7 +423,7 @@ async fn run_guardian_review( target_item_id, turn_id: assessment_turn_id.clone(), started_at_ms, - completed_at_ms: Some(now_unix_timestamp_ms()), + completed_at_ms: Some(completed_at_ms), status: GuardianAssessmentStatus::Aborted, risk_level: None, user_authorization: None, @@ -436,6 +448,7 @@ async fn run_guardian_review( } }; let rationale = format!("Automatic approval review failed: {message}"); + let completed_at_ms = now_unix_timestamp_ms(); track_guardian_review( session.as_ref(), &review_tracking, @@ -445,6 +458,7 @@ async fn run_guardian_review( failure_reason: Some(error.failure_reason()), ..analytics_result }, + completed_at_ms.try_into().unwrap_or_default(), ); ( GuardianAssessment { @@ -454,6 +468,7 @@ async fn run_guardian_review( rationale, }, false, + completed_at_ms, ) } }, @@ -506,7 +521,7 @@ async fn run_guardian_review( target_item_id, turn_id: assessment_turn_id.clone(), started_at_ms, - completed_at_ms: Some(now_unix_timestamp_ms()), + completed_at_ms: Some(completed_at_ms), status, risk_level: Some(assessment.risk_level), user_authorization: Some(assessment.user_authorization), diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 89c12aaf8146..baf46d16915f 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -330,6 +330,7 @@ use codex_protocol::protocol::ErrorEvent; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; +use codex_protocol::protocol::GuardianCommandSource; use codex_protocol::protocol::InitialHistory; use codex_protocol::protocol::McpServerRefreshConfig; use codex_protocol::protocol::ModelRerouteEvent; @@ -1904,6 +1905,7 @@ impl Session { turn_context: &TurnContext, call_id: String, approval_id: Option, + source: GuardianCommandSource, command: Vec, cwd: AbsolutePathBuf, reason: Option, @@ -1957,6 +1959,7 @@ impl Session { approval_id, turn_id: turn_context.sub_id.clone(), started_at_ms: now_unix_timestamp_ms(), + source, command, cwd, reason, diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 14af2c9c5f3c..54b549146531 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -25,6 +25,7 @@ use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::GuardianCommandSource; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::WarningEvent; use indexmap::IndexMap; @@ -519,11 +520,19 @@ impl NetworkApprovalService { .await } else { let available_decisions = None; + let source = match owner_call + .as_ref() + .map(|call| call.trigger.tool_name.as_str()) + { + Some("unified_exec") => GuardianCommandSource::UnifiedExec, + _ => GuardianCommandSource::Shell, + }; session .request_command_approval( turn_context.as_ref(), guardian_approval_id, /*approval_id*/ None, + source, prompt_command, turn_context.cwd.clone(), Some(prompt_reason), diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 7f17285db9d9..7be116a7ca7c 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -38,6 +38,7 @@ use crate::tools::sandboxing::with_cached_approval; use codex_network_proxy::NetworkProxy; use codex_protocol::exec_output::ExecToolCallOutput; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::protocol::GuardianCommandSource; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxablePreference; use codex_shell_command::powershell::prefix_powershell_script_with_utf8; @@ -182,6 +183,7 @@ impl Approvable for ShellRuntime { turn, call_id, /*approval_id*/ None, + GuardianCommandSource::Shell, command, cwd, reason, diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index fef8db5ca9e5..7cdac10f9ca1 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -467,6 +467,7 @@ impl CoreShellActionProvider { &turn, call_id, approval_id, + self.tool_name, command, workdir.clone(), /*reason*/ None, diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 42f311bfcb68..12a8230ddc03 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -42,6 +42,7 @@ use codex_network_proxy::NetworkProxy; use codex_protocol::error::CodexErr; use codex_protocol::error::SandboxErr; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::protocol::GuardianCommandSource; use codex_protocol::protocol::ReviewDecision; use codex_sandboxing::SandboxablePreference; use codex_shell_command::powershell::prefix_powershell_script_with_utf8; @@ -178,6 +179,7 @@ impl Approvable for UnifiedExecRuntime<'_> { turn, call_id, /*approval_id*/ None, + GuardianCommandSource::UnifiedExec, command, cwd.clone(), reason, diff --git a/codex-rs/mcp-server/src/codex_tool_runner.rs b/codex-rs/mcp-server/src/codex_tool_runner.rs index 4e2d5c08e5c6..153aae988a26 100644 --- a/codex-rs/mcp-server/src/codex_tool_runner.rs +++ b/codex-rs/mcp-server/src/codex_tool_runner.rs @@ -223,6 +223,7 @@ async fn run_codex_tool_session_inner( let ExecApprovalRequestEvent { turn_id: _, started_at_ms: _, + source: _, command, cwd, call_id, diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index ace096359c2b..a1695b614d6a 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -131,6 +131,10 @@ pub enum GuardianCommandSource { UnifiedExec, } +fn default_guardian_command_source() -> GuardianCommandSource { + GuardianCommandSource::Shell +} + #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, JsonSchema, TS)] #[serde(tag = "type", rename_all = "snake_case")] #[ts(tag = "type", rename_all = "snake_case")] @@ -231,6 +235,11 @@ pub struct ExecApprovalRequestEvent { pub turn_id: String, #[ts(type = "number")] pub started_at_ms: i64, + /// Originating command tool for this approval request. + /// + /// Uses `#[serde(default)]` for backwards compatibility with older senders. + #[serde(default = "default_guardian_command_source")] + pub source: GuardianCommandSource, /// The command to be executed. pub command: Vec, /// The command's working directory. diff --git a/codex-rs/tui/src/app/app_server_requests.rs b/codex-rs/tui/src/app/app_server_requests.rs index dce87f367ede..45888f459b5a 100644 --- a/codex-rs/tui/src/app/app_server_requests.rs +++ b/codex-rs/tui/src/app/app_server_requests.rs @@ -431,6 +431,7 @@ mod tests { item_id: "call-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: None, network_approval_context: None, command: Some("ls".to_string()), @@ -720,6 +721,7 @@ mod tests { item_id: "call-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: None, network_approval_context: None, command: Some("ls".to_string()), diff --git a/codex-rs/tui/src/app/pending_interactive_replay.rs b/codex-rs/tui/src/app/pending_interactive_replay.rs index 1a21d4df50e3..4135c3f54b2c 100644 --- a/codex-rs/tui/src/app/pending_interactive_replay.rs +++ b/codex-rs/tui/src/app/pending_interactive_replay.rs @@ -614,6 +614,7 @@ mod tests { item_id: call_id.to_string(), started_at_ms: 0, approval_id: approval_id.map(str::to_string), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: None, network_approval_context: None, command: Some("echo hi".to_string()), diff --git a/codex-rs/tui/src/app/tests.rs b/codex-rs/tui/src/app/tests.rs index eacb6d505379..ff2550e94135 100644 --- a/codex-rs/tui/src/app/tests.rs +++ b/codex-rs/tui/src/app/tests.rs @@ -4263,6 +4263,7 @@ fn exec_approval_request( item_id: item_id.to_string(), started_at_ms: 0, approval_id: approval_id.map(str::to_string), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: Some("needs approval".to_string()), network_approval_context: None, command: Some("echo hello".to_string()), diff --git a/codex-rs/tui/src/app/thread_events.rs b/codex-rs/tui/src/app/thread_events.rs index 431bf5f804cb..9380e0545606 100644 --- a/codex-rs/tui/src/app/thread_events.rs +++ b/codex-rs/tui/src/app/thread_events.rs @@ -467,6 +467,7 @@ mod tests { item_id: item_id.to_string(), started_at_ms: 0, approval_id: approval_id.map(str::to_string), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: Some("needs approval".to_string()), network_approval_context: None, command: Some("echo hello".to_string()), diff --git a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs index 85c8fc6c0320..ec711c5534f0 100644 --- a/codex-rs/tui/src/chatwidget/tests/approval_requests.rs +++ b/codex-rs/tui/src/chatwidget/tests/approval_requests.rs @@ -57,6 +57,7 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: None, network_approval_context: None, command: Some( @@ -96,6 +97,7 @@ fn app_server_exec_approval_request_preserves_permissions_context() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: None, network_approval_context: Some(codex_app_server_protocol::NetworkApprovalContext { host: "example.com".to_string(), diff --git a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs index 0045e7d1261e..0a636b8df4d5 100644 --- a/codex-rs/tui/src/chatwidget/tests/exec_flow.rs +++ b/codex-rs/tui/src/chatwidget/tests/exec_flow.rs @@ -56,6 +56,7 @@ fn app_server_exec_approval_request_splits_shell_wrapped_command() { item_id: "item-1".to_string(), started_at_ms: 0, approval_id: Some("approval-1".to_string()), + source: codex_app_server_protocol::GuardianCommandSource::Shell, reason: None, network_approval_context: None, command: Some(