mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[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
This commit is contained in:
committed by
GitHub
Unverified
parent
fe04d75e0f
commit
7f53e47250
@@ -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(
|
||||
|
||||
@@ -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<String>,
|
||||
command_display: String,
|
||||
cwd: String,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
additional_permissions: Option<PermissionProfile>,
|
||||
justification: Option<String>,
|
||||
},
|
||||
UnifiedExec {
|
||||
command: Vec<String>,
|
||||
command_display: String,
|
||||
cwd: String,
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
additional_permissions: Option<PermissionProfile>,
|
||||
justification: Option<String>,
|
||||
tty: bool,
|
||||
},
|
||||
Execve {
|
||||
source: GuardianCommandSource,
|
||||
program: String,
|
||||
argv: Vec<String>,
|
||||
cwd: String,
|
||||
additional_permissions: Option<PermissionProfile>,
|
||||
},
|
||||
ApplyPatch {
|
||||
cwd: String,
|
||||
files: Vec<String>,
|
||||
},
|
||||
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<String>,
|
||||
pub target_item_id: Option<String>,
|
||||
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<GuardianReviewFailureReason>,
|
||||
pub risk_level: Option<GuardianReviewRiskLevel>,
|
||||
pub user_authorization: Option<GuardianReviewUserAuthorization>,
|
||||
pub outcome: Option<GuardianReviewOutcome>,
|
||||
pub rationale: Option<String>,
|
||||
pub risk_level: Option<GuardianRiskLevel>,
|
||||
pub user_authorization: Option<GuardianUserAuthorization>,
|
||||
pub outcome: Option<GuardianAssessmentOutcome>,
|
||||
pub guardian_thread_id: Option<String>,
|
||||
pub guardian_session_kind: Option<GuardianReviewSessionKind>,
|
||||
pub guardian_model: Option<String>,
|
||||
pub guardian_reasoning_effort: Option<String>,
|
||||
pub had_prior_review_context: Option<bool>,
|
||||
pub review_timeout_ms: u64,
|
||||
pub tool_call_count: u64,
|
||||
pub tool_call_count: Option<u64>,
|
||||
pub time_to_first_token_ms: Option<u64>,
|
||||
pub completion_latency_ms: Option<u64>,
|
||||
pub started_at: u64,
|
||||
@@ -330,12 +291,12 @@ pub(crate) struct CodexCompactionEventParams {
|
||||
pub(crate) thread_source: Option<&'static str>,
|
||||
pub(crate) subagent_source: Option<String>,
|
||||
pub(crate) parent_thread_id: Option<String>,
|
||||
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<String>,
|
||||
pub(crate) active_context_tokens_before: i64,
|
||||
pub(crate) active_context_tokens_after: i64,
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user