From f4e6cb78760af4eb75bb370f0f15bd8ca4cb1d3a Mon Sep 17 00:00:00 2001 From: Alec Barber Date: Tue, 1 Sep 2026 15:45:24 +0000 Subject: [PATCH] Attribute Guardian reviews to OpenAI app tools (#42086) ## Why Custom MCP tool names may contain user data, so they should not be added to Guardian metric tags. ## What changed - Add the sanitized `tool` tag to Guardian review counts for MCP calls served by the OpenAI Apps server. - Keep custom MCP calls and Guardian duration and token-usage histograms on their existing tags. ## Testing - Cover network access, OpenAI app tool calls, and custom MCP tool calls in the Guardian metrics test. GitOrigin-RevId: 6bd09ae461eab504f8f07bf7d0c9d1c0aa1317c9 --- codex-rs/core/src/guardian/metrics.rs | 146 ++++++++++++++++++-------- 1 file changed, 105 insertions(+), 41 deletions(-) diff --git a/codex-rs/core/src/guardian/metrics.rs b/codex-rs/core/src/guardian/metrics.rs index 31c69e1ccd..c60c7d961b 100644 --- a/codex-rs/core/src/guardian/metrics.rs +++ b/codex-rs/core/src/guardian/metrics.rs @@ -7,6 +7,7 @@ use codex_analytics::GuardianReviewFailureReason; use codex_analytics::GuardianReviewSessionKind; use codex_analytics::GuardianReviewTerminalStatus; use codex_analytics::GuardianReviewedAction; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_otel::GUARDIAN_REVIEW_COUNT_METRIC; use codex_otel::GUARDIAN_REVIEW_DURATION_METRIC; use codex_otel::GUARDIAN_REVIEW_TOKEN_USAGE_METRIC; @@ -31,7 +32,23 @@ pub(crate) fn emit_guardian_review_metrics( .map(|(key, value)| (*key, value.as_str())) .collect(); - session_telemetry.counter(GUARDIAN_REVIEW_COUNT_METRIC, /*inc*/ 1, &tag_refs); + // Custom MCP tool names may contain user data; only attribute tools from the OpenAI Apps server. + let mcp_tags = if let GuardianReviewedAction::McpToolCall { + server, tool_name, .. + } = reviewed_action + && server == CODEX_APPS_MCP_SERVER_NAME + { + vec![("tool", sanitize_metric_tag_value(tool_name))] + } else { + Vec::new() + }; + let mut counter_tag_refs = tag_refs.clone(); + counter_tag_refs.extend(mcp_tags.iter().map(|(key, value)| (*key, value.as_str()))); + session_telemetry.counter( + GUARDIAN_REVIEW_COUNT_METRIC, + /*inc*/ 1, + &counter_tag_refs, + ); session_telemetry.record_duration( GUARDIAN_REVIEW_DURATION_METRIC, Duration::from_millis(completion_latency_ms), @@ -252,6 +269,7 @@ mod tests { use opentelemetry_sdk::metrics::data::ResourceMetrics; use pretty_assertions::assert_eq; use std::collections::BTreeMap; + use test_case::test_case; fn test_session_telemetry() -> SessionTelemetry { let exporter = InMemoryMetricExporter::default(); @@ -313,21 +331,23 @@ mod tests { } } - fn histogram_sums(resource_metrics: &ResourceMetrics, name: &str) -> BTreeMap { + fn histogram_sums( + resource_metrics: &ResourceMetrics, + name: &str, + expected_base_tags: &BTreeMap, + ) -> BTreeMap { let metric = find_metric(resource_metrics, name); match metric.data() { AggregatedMetrics::F64(data) => match data { MetricData::Histogram(histogram) => histogram .data_points() .map(|point| { - let attrs = attributes_to_map(point.attributes()); - ( - attrs - .get("token_type") - .cloned() - .unwrap_or_else(|| "sample".to_string()), - point.sum() as u64, - ) + let mut attrs = attributes_to_map(point.attributes()); + let token_type = attrs + .remove("token_type") + .unwrap_or_else(|| "sample".to_string()); + assert_eq!(&attrs, expected_base_tags); + (token_type, point.sum() as u64) }) .collect(), _ => panic!("unexpected histogram aggregation"), @@ -336,8 +356,44 @@ mod tests { } } - #[test] - fn guardian_review_metrics_record_counts_durations_and_token_usage() { + #[test_case( + GuardianReviewedAction::NetworkAccess { + protocol: codex_protocol::approvals::NetworkApprovalProtocol::Https, + port: 443, + }, + "network_access", + BTreeMap::new(); + "network access" + )] + #[test_case( + GuardianReviewedAction::McpToolCall { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool_name: "search docs".to_string(), + connector_id: None, + connector_name: None, + tool_title: None, + }, + "mcp_tool_call", + BTreeMap::from([("tool".to_string(), "search_docs".to_string())]); + "openai apps tool call" + )] + #[test_case( + GuardianReviewedAction::McpToolCall { + server: "codex_apps_custom".to_string(), + tool_name: "search_private_docs".to_string(), + connector_id: None, + connector_name: None, + tool_title: None, + }, + "mcp_tool_call", + BTreeMap::new(); + "custom mcp tool call" + )] + fn guardian_review_metrics_record_counts_durations_and_token_usage( + reviewed_action: GuardianReviewedAction, + expected_action: &str, + expected_mcp_tags: BTreeMap, + ) { let session_telemetry = test_session_telemetry(); let result = GuardianReviewAnalyticsResult { decision: GuardianReviewDecision::Approved, @@ -367,10 +423,7 @@ mod tests { &session_telemetry, &result, GuardianApprovalRequestSource::DelegatedSubagent, - &GuardianReviewedAction::NetworkAccess { - protocol: codex_protocol::approvals::NetworkApprovalProtocol::Https, - port: 443, - }, + &reviewed_action, /*completion_latency_ms*/ 456, ); @@ -379,31 +432,34 @@ mod tests { .expect("runtime metrics snapshot"); let (attrs, value) = counter_point(&snapshot, GUARDIAN_REVIEW_COUNT_METRIC); - assert_eq!(value, 1); - assert_eq!( - attrs, - BTreeMap::from([ - ("action".to_string(), "network_access".to_string()), - ( - "approval_request_source".to_string(), - "delegated_subagent".to_string() - ), - ("decision".to_string(), "approved".to_string()), - ("failure_reason".to_string(), "none".to_string()), - ("guardian_model".to_string(), "gpt-5.4_guardian".to_string()), - ("guardian_reasoning_effort".to_string(), "low".to_string()), - ("had_prior_review_context".to_string(), "true".to_string()), - ("outcome".to_string(), "allow".to_string()), - ("reviewed_action_truncated".to_string(), "true".to_string()), - ("risk_level".to_string(), "low".to_string()), - ("session_kind".to_string(), "trunk_reused".to_string()), - ("terminal_status".to_string(), "approved".to_string()), - ("user_authorization".to_string(), "high".to_string()), - ]) - ); + let expected_base_tags = BTreeMap::from([ + ("action".to_string(), expected_action.to_string()), + ( + "approval_request_source".to_string(), + "delegated_subagent".to_string(), + ), + ("decision".to_string(), "approved".to_string()), + ("failure_reason".to_string(), "none".to_string()), + ("guardian_model".to_string(), "gpt-5.4_guardian".to_string()), + ("guardian_reasoning_effort".to_string(), "low".to_string()), + ("had_prior_review_context".to_string(), "true".to_string()), + ("outcome".to_string(), "allow".to_string()), + ("reviewed_action_truncated".to_string(), "true".to_string()), + ("risk_level".to_string(), "low".to_string()), + ("session_kind".to_string(), "trunk_reused".to_string()), + ("terminal_status".to_string(), "approved".to_string()), + ("user_authorization".to_string(), "high".to_string()), + ]); + let mut expected_counter_tags = expected_base_tags.clone(); + expected_counter_tags.extend(expected_mcp_tags); + assert_eq!((attrs, value), (expected_counter_tags, 1)); assert_eq!( - histogram_sums(&snapshot, GUARDIAN_REVIEW_TOKEN_USAGE_METRIC), + histogram_sums( + &snapshot, + GUARDIAN_REVIEW_TOKEN_USAGE_METRIC, + &expected_base_tags + ), BTreeMap::from([ ("cached_input".to_string(), 4), ("cache_write_input".to_string(), 2), @@ -415,11 +471,19 @@ mod tests { ]) ); assert_eq!( - histogram_sums(&snapshot, GUARDIAN_REVIEW_DURATION_METRIC), + histogram_sums( + &snapshot, + GUARDIAN_REVIEW_DURATION_METRIC, + &expected_base_tags + ), BTreeMap::from([("sample".to_string(), 456)]) ); assert_eq!( - histogram_sums(&snapshot, GUARDIAN_REVIEW_TTFT_DURATION_METRIC), + histogram_sums( + &snapshot, + GUARDIAN_REVIEW_TTFT_DURATION_METRIC, + &expected_base_tags + ), BTreeMap::from([("sample".to_string(), 123)]) ); }