From 7f53e4725062d9bdfef568a63d4c1bfb79998370 Mon Sep 17 00:00:00 2001 From: rhan-oai Date: Mon, 20 Apr 2026 13:08:17 -0700 Subject: [PATCH] [codex-analytics] guardian review analytics schema polishing (#17692) ## Why Guardian review analytics needs a Rust event shape that matches the backend schema while avoiding unnecessary PII exposure from reviewed tool calls. This PR narrows the analytics payload to the fields we intend to emit and keeps shared Guardian assessment enums in protocol instead of duplicating equivalent analytics-only enums. ## What changed - Uses protocol Guardian enums directly for `risk_level`, `user_authorization`, `outcome`, and command source values. - Removes high-risk reviewed-action fields from the analytics payload, including raw commands, display strings, working directories, file paths, network targets/hosts, justification text, retry reason, and rationale text. - Makes `target_item_id` and `tool_call_count` nullable so the Codex event can represent cases where the app-server protocol or producer does not have those values. - Keeps lower-risk structured reviewed-action metadata such as sandbox permissions, permission profile, `tty`, `execve` source/program, network protocol/port, and MCP connector/tool labels. - Adds an analytics reducer/client test covering `codex_guardian_review` serialization with an optional `target_item_id` and absent removed fields. ## Verification - `cargo test -p codex-analytics guardian_review_event_ingests_custom_fact_with_optional_target_item` - `cargo fmt --check` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/17692). * #17696 * #17695 * #17693 * __->__ #17692 --- .../analytics/src/analytics_client_tests.rs | 136 ++++++++++++++++++ codex-rs/analytics/src/events.rs | 83 +++-------- codex-rs/analytics/src/lib.rs | 4 - codex-rs/core/src/guardian/mod.rs | 9 +- codex-rs/protocol/src/approvals.rs | 8 ++ codex-rs/protocol/src/protocol.rs | 1 + 6 files changed, 168 insertions(+), 73 deletions(-) diff --git a/codex-rs/analytics/src/analytics_client_tests.rs b/codex-rs/analytics/src/analytics_client_tests.rs index 4b13b2819..bf97da428 100644 --- a/codex-rs/analytics/src/analytics_client_tests.rs +++ b/codex-rs/analytics/src/analytics_client_tests.rs @@ -9,6 +9,12 @@ use crate::events::CodexPluginEventRequest; use crate::events::CodexPluginUsedEventRequest; use crate::events::CodexRuntimeMetadata; use crate::events::CodexTurnEventRequest; +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::ThreadInitializedEvent; use crate::events::ThreadInitializedEventParams; use crate::events::TrackEventRequest; @@ -82,6 +88,7 @@ use codex_plugin::AppConnectorId; use codex_plugin::PluginCapabilitySummary; use codex_plugin::PluginId; use codex_plugin::PluginTelemetryMetadata; +use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::ModeKind; use codex_protocol::protocol::AskForApproval; @@ -1050,6 +1057,135 @@ async fn compaction_event_ingests_custom_fact() { assert_eq!(payload[0]["event_params"]["status"], "failed"); } +#[tokio::test] +async fn guardian_review_event_ingests_custom_fact_with_optional_target_item() { + let mut reducer = AnalyticsReducer::default(); + let mut events = Vec::new(); + + reducer + .ingest( + AnalyticsFact::Initialize { + connection_id: 7, + params: InitializeParams { + client_info: ClientInfo { + name: "codex-tui".to_string(), + title: None, + version: "1.0.0".to_string(), + }, + capabilities: Some(InitializeCapabilities { + experimental_api: false, + opt_out_notification_methods: None, + }), + }, + product_client_id: DEFAULT_ORIGINATOR.to_string(), + runtime: sample_runtime_metadata(), + rpc_transport: AppServerRpcTransport::Websocket, + }, + &mut events, + ) + .await; + reducer + .ingest( + AnalyticsFact::Response { + connection_id: 7, + response: Box::new(sample_thread_start_response( + "thread-guardian", + /*ephemeral*/ false, + "gpt-5", + )), + }, + &mut events, + ) + .await; + events.clear(); + + reducer + .ingest( + AnalyticsFact::Custom(CustomAnalyticsFact::GuardianReview(Box::new( + GuardianReviewEventParams { + thread_id: "thread-guardian".to_string(), + turn_id: "turn-guardian".to_string(), + review_id: "review-guardian".to_string(), + target_item_id: None, + approval_request_source: GuardianApprovalRequestSource::DelegatedSubagent, + reviewed_action: GuardianReviewedAction::NetworkAccess { + protocol: NetworkApprovalProtocol::Https, + port: 443, + }, + reviewed_action_truncated: false, + decision: GuardianReviewDecision::Denied, + terminal_status: GuardianReviewTerminalStatus::TimedOut, + failure_reason: Some(GuardianReviewFailureReason::Timeout), + risk_level: None, + user_authorization: None, + outcome: None, + guardian_thread_id: None, + guardian_session_kind: None, + guardian_model: None, + guardian_reasoning_effort: None, + had_prior_review_context: None, + review_timeout_ms: 90_000, + tool_call_count: None, + time_to_first_token_ms: None, + completion_latency_ms: Some(90_000), + started_at: 100, + completed_at: Some(190), + input_tokens: None, + cached_input_tokens: None, + output_tokens: None, + reasoning_output_tokens: None, + total_tokens: None, + }, + ))), + &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_guardian_review"); + assert_eq!(payload[0]["event_params"]["thread_id"], "thread-guardian"); + assert_eq!(payload[0]["event_params"]["turn_id"], "turn-guardian"); + assert_eq!(payload[0]["event_params"]["review_id"], "review-guardian"); + assert_eq!(payload[0]["event_params"]["target_item_id"], json!(null)); + assert_eq!( + payload[0]["event_params"]["approval_request_source"], + "delegated_subagent" + ); + assert_eq!( + payload[0]["event_params"]["app_server_client"]["product_client_id"], + DEFAULT_ORIGINATOR + ); + assert_eq!( + payload[0]["event_params"]["runtime"]["codex_rs_version"], + "0.1.0" + ); + assert_eq!( + payload[0]["event_params"]["reviewed_action"]["type"], + "network_access" + ); + assert_eq!( + payload[0]["event_params"]["reviewed_action"]["protocol"], + "https" + ); + assert_eq!(payload[0]["event_params"]["reviewed_action"]["port"], 443); + assert!(payload[0]["event_params"].get("retry_reason").is_none()); + assert!(payload[0]["event_params"].get("rationale").is_none()); + assert!( + payload[0]["event_params"]["reviewed_action"] + .get("target") + .is_none() + ); + assert!( + payload[0]["event_params"]["reviewed_action"] + .get("host") + .is_none() + ); + assert_eq!(payload[0]["event_params"]["terminal_status"], "timed_out"); + assert_eq!(payload[0]["event_params"]["failure_reason"], "timeout"); + assert_eq!(payload[0]["event_params"]["review_timeout_ms"], 90_000); +} + #[test] fn subagent_thread_started_review_serializes_expected_shape() { let event = TrackEventRequest::ThreadInitialized(subagent_thread_started_event_request( diff --git a/codex-rs/analytics/src/events.rs b/codex-rs/analytics/src/events.rs index 9119837fa..b542f8268 100644 --- a/codex-rs/analytics/src/events.rs +++ b/codex-rs/analytics/src/events.rs @@ -1,5 +1,11 @@ use crate::facts::AppInvocation; use crate::facts::CodexCompactionEvent; +use crate::facts::CompactionImplementation; +use crate::facts::CompactionPhase; +use crate::facts::CompactionReason; +use crate::facts::CompactionStatus; +use crate::facts::CompactionStrategy; +use crate::facts::CompactionTrigger; use crate::facts::HookRunFact; use crate::facts::InvocationType; use crate::facts::PluginState; @@ -16,6 +22,10 @@ use codex_plugin::PluginTelemetryMetadata; use codex_protocol::approvals::NetworkApprovalProtocol; use codex_protocol::models::PermissionProfile; use codex_protocol::models::SandboxPermissions; +use codex_protocol::protocol::GuardianAssessmentOutcome; +use codex_protocol::protocol::GuardianCommandSource; +use codex_protocol::protocol::GuardianRiskLevel; +use codex_protocol::protocol::GuardianUserAuthorization; use codex_protocol::protocol::HookEventName; use codex_protocol::protocol::HookRunStatus; use codex_protocol::protocol::HookSource; @@ -151,31 +161,6 @@ pub enum GuardianReviewSessionKind { EphemeralForked, } -#[derive(Clone, Copy, Debug, Serialize)] -#[serde(rename_all = "lowercase")] -pub enum GuardianReviewRiskLevel { - Low, - Medium, - High, - Critical, -} - -#[derive(Clone, Copy, Debug, Serialize)] -#[serde(rename_all = "lowercase")] -pub enum GuardianReviewUserAuthorization { - Unknown, - Low, - Medium, - High, -} - -#[derive(Clone, Copy, Debug, Serialize)] -#[serde(rename_all = "lowercase")] -pub enum GuardianReviewOutcome { - Allow, - Deny, -} - #[derive(Clone, Copy, Debug, Serialize)] #[serde(rename_all = "snake_case")] pub enum GuardianApprovalRequestSource { @@ -190,36 +175,21 @@ pub enum GuardianApprovalRequestSource { #[serde(tag = "type", rename_all = "snake_case")] pub enum GuardianReviewedAction { Shell { - command: Vec, - command_display: String, - cwd: String, sandbox_permissions: SandboxPermissions, additional_permissions: Option, - justification: Option, }, UnifiedExec { - command: Vec, - command_display: String, - cwd: String, sandbox_permissions: SandboxPermissions, additional_permissions: Option, - justification: Option, tty: bool, }, Execve { source: GuardianCommandSource, program: String, - argv: Vec, - cwd: String, additional_permissions: Option, }, - ApplyPatch { - cwd: String, - files: Vec, - }, + ApplyPatch {}, NetworkAccess { - target: String, - host: String, protocol: NetworkApprovalProtocol, port: u16, }, @@ -232,37 +202,28 @@ pub enum GuardianReviewedAction { }, } -#[derive(Clone, Copy, Debug, Serialize)] -#[serde(rename_all = "snake_case")] -pub enum GuardianCommandSource { - Shell, - UnifiedExec, -} - #[derive(Clone, Serialize)] pub struct GuardianReviewEventParams { pub thread_id: String, pub turn_id: String, pub review_id: String, - pub target_item_id: String, - pub retry_reason: Option, + pub target_item_id: Option, pub approval_request_source: GuardianApprovalRequestSource, pub reviewed_action: GuardianReviewedAction, pub reviewed_action_truncated: bool, pub decision: GuardianReviewDecision, pub terminal_status: GuardianReviewTerminalStatus, pub failure_reason: Option, - pub risk_level: Option, - pub user_authorization: Option, - pub outcome: Option, - pub rationale: Option, + pub risk_level: Option, + pub user_authorization: Option, + pub outcome: Option, pub guardian_thread_id: Option, pub guardian_session_kind: Option, pub guardian_model: Option, pub guardian_reasoning_effort: Option, pub had_prior_review_context: Option, pub review_timeout_ms: u64, - pub tool_call_count: u64, + pub tool_call_count: Option, pub time_to_first_token_ms: Option, pub completion_latency_ms: Option, pub started_at: u64, @@ -330,12 +291,12 @@ pub(crate) struct CodexCompactionEventParams { pub(crate) thread_source: Option<&'static str>, pub(crate) subagent_source: Option, pub(crate) parent_thread_id: Option, - pub(crate) trigger: crate::facts::CompactionTrigger, - pub(crate) reason: crate::facts::CompactionReason, - pub(crate) implementation: crate::facts::CompactionImplementation, - pub(crate) phase: crate::facts::CompactionPhase, - pub(crate) strategy: crate::facts::CompactionStrategy, - pub(crate) status: crate::facts::CompactionStatus, + pub(crate) trigger: CompactionTrigger, + pub(crate) reason: CompactionReason, + pub(crate) implementation: CompactionImplementation, + pub(crate) phase: CompactionPhase, + pub(crate) strategy: CompactionStrategy, + pub(crate) status: CompactionStatus, pub(crate) error: Option, pub(crate) active_context_tokens_before: i64, pub(crate) active_context_tokens_after: i64, diff --git a/codex-rs/analytics/src/lib.rs b/codex-rs/analytics/src/lib.rs index 03f485a1c..5c4cdfac7 100644 --- a/codex-rs/analytics/src/lib.rs +++ b/codex-rs/analytics/src/lib.rs @@ -9,15 +9,11 @@ use std::time::UNIX_EPOCH; pub use client::AnalyticsEventsClient; pub use events::AppServerRpcTransport; pub use events::GuardianApprovalRequestSource; -pub use events::GuardianCommandSource; pub use events::GuardianReviewDecision; pub use events::GuardianReviewEventParams; pub use events::GuardianReviewFailureReason; -pub use events::GuardianReviewOutcome; -pub use events::GuardianReviewRiskLevel; pub use events::GuardianReviewSessionKind; pub use events::GuardianReviewTerminalStatus; -pub use events::GuardianReviewUserAuthorization; pub use events::GuardianReviewedAction; pub use facts::AnalyticsJsonRpcError; pub use facts::AppInvocation; diff --git a/codex-rs/core/src/guardian/mod.rs b/codex-rs/core/src/guardian/mod.rs index 4fa150a23..f31f21344 100644 --- a/codex-rs/core/src/guardian/mod.rs +++ b/codex-rs/core/src/guardian/mod.rs @@ -19,6 +19,7 @@ mod review_session; use std::time::Duration; use codex_protocol::protocol::GuardianAssessmentDecisionSource; +use codex_protocol::protocol::GuardianAssessmentOutcome; use serde::Deserialize; use serde::Serialize; @@ -45,14 +46,6 @@ const GUARDIAN_MAX_ACTION_STRING_TOKENS: usize = 16_000; const GUARDIAN_RECENT_ENTRY_LIMIT: usize = 40; const TRUNCATION_TAG: &str = "truncated"; -/// Final allow/deny outcome returned by the guardian reviewer. -#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq)] -#[serde(rename_all = "lowercase")] -pub(crate) enum GuardianAssessmentOutcome { - Allow, - Deny, -} - /// Structured output contract that the guardian reviewer must satisfy. #[derive(Debug, Clone, Deserialize, Serialize)] pub(crate) struct GuardianAssessment { diff --git a/codex-rs/protocol/src/approvals.rs b/codex-rs/protocol/src/approvals.rs index db00db9b3..b8839b51b 100644 --- a/codex-rs/protocol/src/approvals.rs +++ b/codex-rs/protocol/src/approvals.rs @@ -100,6 +100,14 @@ pub enum GuardianUserAuthorization { High, } +/// Final allow/deny outcome returned by the guardian reviewer. +#[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] +#[serde(rename_all = "lowercase")] +pub enum GuardianAssessmentOutcome { + Allow, + Deny, +} + #[derive(Debug, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)] #[serde(rename_all = "snake_case")] pub enum GuardianAssessmentStatus { diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index ec1fc847a..475d2941c 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -66,6 +66,7 @@ pub use crate::approvals::ExecPolicyAmendment; pub use crate::approvals::GuardianAssessmentAction; pub use crate::approvals::GuardianAssessmentDecisionSource; pub use crate::approvals::GuardianAssessmentEvent; +pub use crate::approvals::GuardianAssessmentOutcome; pub use crate::approvals::GuardianAssessmentStatus; pub use crate::approvals::GuardianCommandSource; pub use crate::approvals::GuardianRiskLevel;