openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-19 17:28:46 +00:00
Code Issues Packages Projects Releases Wiki Activity

Add Guardian v2 approval review metrics (#39224)

Browse Source 
## What changed

- Pass metrics bound to the active turn's model to approval-review contributors.
- Add counter support to `ExtensionMetrics` while preserving host session attribution.
- Record Guardian v2 tool-call score lag and count stale-score fallbacks with the `score_lag` reason.

## Testing

- Cover counter forwarding and session metadata tags.
- Verify Guardian v2 metric samples for current and stale scores.

GitOrigin-RevId: 0ec34736d29d2e37ed217a27e773b4f6230ae38a
This commit is contained in:
Dylan Hurd
2026-08-18 16:27:54 +00:00
committed by copyberry
parent 2a30972fcb
commit e7e13c68e2
15 changed files with 230 additions and 16 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/ext/extension-api/src/capabilities/metrics.rs
+3
View File
@@ -3,6 +3,9 @@
/// Implementations are expected to attach the host's session attribution before
/// forwarding samples to the configured metrics backend.
pub trait ExtensionMetrics: Send + Sync {
/// Increments a counter with optional extension-provided tags.
fn counter(&self, name: &str, inc: i64, tags: &[(&str, &str)]);
/// Records one histogram sample with optional extension-provided tags.
fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]);
}
codex-rs/ext/extension-api/src/contributors.rs
+2
View File
@@ -318,11 +318,13 @@ pub trait ToolLifecycleContributor: Send + Sync {
/// Extension contribution that can claim rendered approval-review prompts.
pub trait ApprovalReviewContributor: Send + Sync {
/// Reviews one action using metrics bound to the active turn's model.
fn contribute<'a>(
&'a self,
session_store: &'a ExtensionData,
thread_store: &'a ExtensionData,
prompt: &'a str,
extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>>;
}
codex-rs/ext/extension-api/src/registry.rs
+8 -1
View File
@@ -7,6 +7,7 @@ use crate::ConfigContributor;
use crate::ContextContributor;
use crate::ExtensionData;
use crate::ExtensionEventSink;
use crate::ExtensionMetrics;
use crate::McpServerContributor;
use crate::NoopExtensionEventSink;
use crate::SkillInvocationContributor;
@@ -194,10 +195,16 @@ impl<C: Sync> ExtensionRegistry<C> {
session_store: &ExtensionData,
thread_store: &ExtensionData,
prompt: &str,
extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> Option<ReviewDecision> {
for contributor in &self.approval_review_contributors {
if let Some(decision) = contributor
.contribute(session_store, thread_store, prompt)
.contribute(
session_store,
thread_store,
prompt,
extension_metrics.clone(),
)
.await
{
return Some(decision);
codex-rs/ext/extension-api/tests/registry.rs
+5
View File
@@ -135,6 +135,7 @@ impl ApprovalReviewContributor for AllContributors {
_session_store: &'a ExtensionData,
_thread_store: &'a ExtensionData,
_prompt: &'a str,
_extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move {
let _self = self;
@@ -176,6 +177,7 @@ async fn build_round_trips_every_contributor_category() {
&ExtensionData::new("session"),
&ExtensionData::new("thread"),
"review this",
/*extension_metrics*/ None,
)
.await,
Some(ReviewDecision::ApprovedForSession)
@@ -322,6 +324,7 @@ impl ApprovalReviewContributor for RecordingApprovalContributor {
session_store: &'a ExtensionData,
thread_store: &'a ExtensionData,
prompt: &'a str,
_extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move {
self.calls
@@ -363,6 +366,7 @@ async fn approval_review_returns_first_claim_and_short_circuits() {
&ExtensionData::new("session-1"),
&ExtensionData::new("thread-1"),
"allow command?",
/*extension_metrics*/ None,
)
.await;
@@ -451,6 +455,7 @@ async fn empty_registry_does_not_claim_approval_review() {
&ExtensionData::new("session"),
&ExtensionData::new("thread"),
"unclaimed",
/*extension_metrics*/ None,
)
.await,
None
codex-rs/ext/guardian-v2/src/extension.rs
+20
View File
@@ -11,6 +11,7 @@ use codex_extension_api::ApprovalReviewContributor;
use codex_extension_api::ExtensionData;
use codex_extension_api::ExtensionEventSink;
use codex_extension_api::ExtensionFuture;
use codex_extension_api::ExtensionMetrics;
use codex_extension_api::ExtensionRegistryBuilder;
use codex_extension_api::ExtensionWarning;
use codex_extension_api::ResponseItem;
@@ -190,6 +191,10 @@ pub enum StrictReviewReason {
struct GuardianV2Enabled;
// Sampled when the approval contributor evaluates the cached Luna score.
const TOOL_CALL_LAG_METRIC: &str = "codex.guardian_v2.tool_call_lag";
const REVIEW_FALLBACK_METRIC: &str = "codex.guardian_v2.review_fallback";
#[derive(Default)]
struct GuardianV2ScoreProgress {
latest_tool_call: AtomicUsize,
@@ -284,6 +289,7 @@ impl ApprovalReviewContributor for GuardianV2Extension {
_session_store: &'a ExtensionData,
thread_store: &'a ExtensionData,
_prompt: &'a str,
extension_metrics: Option<Arc<dyn ExtensionMetrics>>,
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async move {
thread_store.get::<GuardianV2Enabled>()?;
@@ -297,8 +303,22 @@ impl ApprovalReviewContributor for GuardianV2Extension {
.latest_scored_tool_call
.load(Ordering::Acquire),
);
if let Some(metrics) = &extension_metrics {
metrics.histogram(
TOOL_CALL_LAG_METRIC,
i64::try_from(tool_call_lag).unwrap_or(i64::MAX),
&[],
);
}
if tool_call_lag > guardian_config.max_tool_call_lag {
thread_store.insert(StrictReviewReason::StaleScore);
if let Some(metrics) = &extension_metrics {
metrics.counter(
REVIEW_FALLBACK_METRIC,
/*inc*/ 1,
&[("fallback_reason", "score_lag")],
);
}
return None;
}
codex-rs/ext/guardian-v2/src/extension_tests.rs
+144 -13
View File
@@ -1,5 +1,6 @@
use std::collections::BTreeMap;
use std::sync::Arc;
use std::sync::Mutex;
use std::sync::atomic::Ordering;
use std::time::Duration;
@@ -7,6 +8,7 @@ use anyhow::Result;
use codex_core::config::Config;
use codex_extension_api::ConversationHistorySnapshot;
use codex_extension_api::ExtensionData;
use codex_extension_api::ExtensionMetrics;
use codex_extension_api::ExtensionRegistry;
use codex_extension_api::ExtensionRegistryBuilder;
use codex_extension_api::ResponseItem;
@@ -47,7 +49,9 @@ use pretty_assertions::assert_eq;
use serde_json::json;
use super::GuardianV2ScoreProgress;
use super::REVIEW_FALLBACK_METRIC;
use super::StrictReviewReason;
use super::TOOL_CALL_LAG_METRIC;
use super::encrypted_parent_compaction;
use crate::config::DEFAULT_MODEL_CONTEXT_ITEM_TOKENS;
use crate::config::DEFAULT_PARENT_COMPACTION_TOKENS;
@@ -177,6 +181,37 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> {
Ok(())
}
#[derive(Debug, PartialEq)]
enum RecordedMetric {
Histogram(String, i64, Vec<(String, String)>),
Counter(String, i64, Vec<(String, String)>),
}
#[derive(Default)]
struct RecordingMetrics(Mutex<Vec<RecordedMetric>>);
impl ExtensionMetrics for RecordingMetrics {
fn counter(&self, name: &str, inc: i64, tags: &[(&str, &str)]) {
self.0.lock().unwrap().push(RecordedMetric::Counter(
name.to_owned(),
inc,
tags.iter()
.map(|(key, value)| ((*key).to_owned(), (*value).to_owned()))
.collect(),
));
}
fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) {
self.0.lock().unwrap().push(RecordedMetric::Histogram(
name.to_owned(),
value,
tags.iter()
.map(|(key, value)| ((*key).to_owned(), (*value).to_owned()))
.collect(),
));
}
}
struct TestConversationHistory(Vec<ResponseItem>);
impl ConversationHistorySnapshot for TestConversationHistory {
@@ -425,6 +460,8 @@ async fn sample_configured_conversation_history(
let registry = builder.build();
let session_store = ExtensionData::new("session-1");
let thread_store = test.codex.thread_extension_data();
thread_store.insert(RecordingMetrics::default());
let metrics = thread_store.get::<RecordingMetrics>().unwrap();
if has_model_defaults {
let parent_model = test
.thread_manager
@@ -435,7 +472,12 @@ async fn sample_configured_conversation_history(
}
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
/*extension_metrics*/ None,
)
.await,
None
);
@@ -446,7 +488,7 @@ async fn sample_configured_conversation_history(
persistent_thread_state_available: false,
environments: &[],
mcp_resource_client: None,
extension_metrics: None,
extension_metrics: Some(metrics),
session_store: &session_store,
thread_store,
})
@@ -607,7 +649,14 @@ max_recent_non_user_entries = 8
});
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
thread_store
.get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
)
.await,
None
);
@@ -621,7 +670,14 @@ max_recent_non_user_entries = 8
});
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
thread_store
.get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
)
.await,
Some(ReviewDecision::Approved)
);
@@ -637,17 +693,33 @@ max_recent_non_user_entries = 8
.store(/*val*/ 3, Ordering::Release);
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
thread_store
.get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
)
.await,
Some(ReviewDecision::Approved)
);
let initial_metrics = thread_store.get::<RecordingMetrics>().unwrap();
thread_store.insert(RecordingMetrics::default());
score_progress
.latest_tool_call
.store(/*val*/ 4, Ordering::Release);
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
thread_store
.get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
)
.await,
None
);
@@ -661,11 +733,40 @@ max_recent_non_user_entries = 8
.store(/*val*/ 2, Ordering::Release);
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
thread_store
.get::<RecordingMetrics>()
.map(|metrics| metrics as Arc<dyn ExtensionMetrics>),
)
.await,
Some(ReviewDecision::Approved)
);
assert_eq!(
*initial_metrics.0.lock().unwrap(),
vec![
RecordedMetric::Histogram(TOOL_CALL_LAG_METRIC.to_owned(), 0, vec![]),
RecordedMetric::Histogram(TOOL_CALL_LAG_METRIC.to_owned(), 0, vec![]),
RecordedMetric::Histogram(TOOL_CALL_LAG_METRIC.to_owned(), 2, vec![]),
]
);
let metrics = thread_store.get::<RecordingMetrics>().unwrap();
assert_eq!(
*metrics.0.lock().unwrap(),
vec![
RecordedMetric::Histogram(TOOL_CALL_LAG_METRIC.to_owned(), 3, vec![]),
RecordedMetric::Counter(
REVIEW_FALLBACK_METRIC.to_owned(),
1,
vec![("fallback_reason".to_owned(), "score_lag".to_owned())],
),
RecordedMetric::Histogram(TOOL_CALL_LAG_METRIC.to_owned(), 2, vec![]),
]
);
Ok(())
}
@@ -847,7 +948,12 @@ async fn contributor_uses_model_defaults_and_preserves_local_overrides() -> Resu
});
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
/*extension_metrics*/ None,
)
.await,
Some(ReviewDecision::Approved)
);
@@ -980,7 +1086,12 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
assert!(score.sampled_at.is_some());
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
/*extension_metrics*/ None,
)
.await,
None
);
@@ -1005,7 +1116,12 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
});
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
/*extension_metrics*/ None,
)
.await,
Some(ReviewDecision::Approved)
);
@@ -1017,7 +1133,12 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
});
assert_eq!(
registry
.approval_review(&session_store, &disabled_thread_store, "review action")
.approval_review(
&session_store,
&disabled_thread_store,
"review action",
/*extension_metrics*/ None
)
.await,
None
);
@@ -1309,7 +1430,12 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result<
assert_eq!(previous_score.scores.get("action_risk"), Some(&0.25));
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
/*extension_metrics*/ None,
)
.await,
Some(ReviewDecision::Approved)
);
@@ -1348,7 +1474,12 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result<
);
assert_eq!(
registry
.approval_review(&session_store, thread_store, "review action")
.approval_review(
&session_store,
thread_store,
"review action",
/*extension_metrics*/ None,
)
.await,
None
);
codex-rs/ext/skills/src/extension_tests.rs
+4
View File
@@ -15,6 +15,10 @@ struct RecordingMetrics {
}
impl ExtensionMetrics for RecordingMetrics {
fn counter(&self, name: &str, _inc: i64, _tags: &[(&str, &str)]) {
panic!("unexpected counter: {name}");
}
fn histogram(&self, name: &str, value: i64, _tags: &[(&str, &str)]) {
self.samples
.lock()
codex-rs/ext/skills/src/render_observability_tests.rs
+4
View File
@@ -18,6 +18,10 @@ struct RecordingMetrics {
}
impl ExtensionMetrics for RecordingMetrics {
fn counter(&self, name: &str, _inc: i64, _tags: &[(&str, &str)]) {
panic!("unexpected counter: {name}");
}
fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) {
self.samples
.lock()
codex-rs/ext/skills/tests/skills_extension.rs
+4
View File
@@ -2294,6 +2294,10 @@ impl RecordingMetrics {
}
impl ExtensionMetrics for RecordingMetrics {
fn counter(&self, name: &str, _inc: i64, _tags: &[(&str, &str)]) {
panic!("unexpected counter: {name}");
}
fn histogram(&self, name: &str, value: i64, tags: &[(&str, &str)]) {
self.samples
.lock()