mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Track Guardian reviewer turn and tool analytics (#41023)
## Why Trusted Guardian reviewer sessions can emit events without an app-server listener, leaving their turns and tool usage absent from analytics. ## What changed - Translate Guardian session lifecycle and canonical tool item events into analytics notifications while excluding messages, reasoning, and review content. - Preserve Guardian lineage during prewarming even when inherited client metadata is not yet available, without emitting an incomplete thread initialization event. - Limit direct session tracking to trusted Guardian children so resumed threads that have an app-server listener do not report duplicate events. ## Testing - Cover completed, failed, and interrupted Guardian turns, tool counts and timing, lineage, content exclusion, prewarmed lineage, and resumed-thread deduplication. GitOrigin-RevId: 70ace5f7ed30a7e3b6a9d6097c8b2a6a326dc4f7
This commit is contained in:
committed by
copyberry
parent
81e180044d
commit
307ce6cda9
@@ -1993,6 +1993,24 @@ impl Session {
|
||||
id: turn_context.sub_id.clone(),
|
||||
msg,
|
||||
};
|
||||
// Private reviewers have no app-server listener; publicly resumed Guardian threads do.
|
||||
if matches!(
|
||||
&turn_context.session_source,
|
||||
SessionSource::SubAgent(SubAgentSource::Other(name))
|
||||
if name == crate::guardian::GUARDIAN_REVIEWER_NAME
|
||||
) && self.services.analytics_events_client.is_enabled()
|
||||
&& turn_context.parent_thread_id.is_some()
|
||||
&& self
|
||||
.state
|
||||
.lock()
|
||||
.await
|
||||
.session_configuration
|
||||
.trusted_guardian_reviewer
|
||||
{
|
||||
self.services
|
||||
.analytics_events_client
|
||||
.track_guardian_session_event(self.thread_id, &event);
|
||||
}
|
||||
self.send_event_raw(event).await;
|
||||
self.maybe_notify_parent_of_terminal_turn(turn_context, &legacy_source)
|
||||
.await;
|
||||
@@ -4329,10 +4347,12 @@ pub(crate) fn emit_subagent_session_started(
|
||||
client_name,
|
||||
client_version,
|
||||
} = client_metadata;
|
||||
let (Some(client_name), Some(client_version)) = (client_name, client_version) else {
|
||||
if (client_name.is_none() || client_version.is_none())
|
||||
&& subagent_source.kind() != crate::guardian::GUARDIAN_REVIEWER_NAME
|
||||
{
|
||||
tracing::warn!("skipping subagent thread analytics: missing inherited client metadata");
|
||||
return;
|
||||
};
|
||||
}
|
||||
let created_at = SystemTime::now()
|
||||
.duration_since(UNIX_EPOCH)
|
||||
.unwrap_or_default()
|
||||
|
||||
@@ -5151,6 +5151,8 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati
|
||||
|
||||
#[tokio::test]
|
||||
async fn emit_subagent_session_started_includes_fork_lineage_and_originator() {
|
||||
use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ThreadArchivedNotification;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
@@ -5226,6 +5228,59 @@ async fn emit_subagent_session_started_includes_fork_lineage_and_originator() {
|
||||
event["event_params"]["app_server_client"]["product_client_id"],
|
||||
"test_originator"
|
||||
);
|
||||
|
||||
let prewarmed_thread_id = ThreadId::new();
|
||||
emit_subagent_session_started(
|
||||
&analytics_events_client,
|
||||
AppServerClientMetadata {
|
||||
client_name: None,
|
||||
client_version: None,
|
||||
},
|
||||
SessionId::from(parent_thread_id),
|
||||
prewarmed_thread_id,
|
||||
Some(parent_thread_id),
|
||||
session_configuration.thread_config_snapshot(Vec::new()),
|
||||
SubAgentSource::Other(crate::guardian::GUARDIAN_REVIEWER_NAME.to_string()),
|
||||
);
|
||||
// Archive analytics exposes retained lineage even before a parent connection exists.
|
||||
analytics_events_client.track_notification(&ServerNotification::ThreadArchived(
|
||||
ThreadArchivedNotification {
|
||||
thread_id: prewarmed_thread_id.to_string(),
|
||||
},
|
||||
));
|
||||
analytics_events_client.flush().await;
|
||||
let events = server
|
||||
.received_requests()
|
||||
.await
|
||||
.expect("analytics requests")
|
||||
.into_iter()
|
||||
.flat_map(|request| {
|
||||
let payload: serde_json::Value =
|
||||
serde_json::from_slice(&request.body).expect("valid analytics payload");
|
||||
payload["events"]
|
||||
.as_array()
|
||||
.expect("analytics events")
|
||||
.clone()
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
let [initialization, archive] = events.as_slice() else {
|
||||
panic!("expected one complete initialization and one archive: {events:?}");
|
||||
};
|
||||
assert_eq!(initialization, &event);
|
||||
assert_eq!(
|
||||
json!([
|
||||
archive["event_type"],
|
||||
archive["event_params"]["thread_id"],
|
||||
archive["event_params"]["thread_source"],
|
||||
archive["event_params"]["parent_thread_id"],
|
||||
]),
|
||||
json!([
|
||||
"codex_thread_archive_event",
|
||||
prewarmed_thread_id.to_string(),
|
||||
"guardian_review",
|
||||
parent_thread_id.to_string(),
|
||||
])
|
||||
);
|
||||
}
|
||||
|
||||
async fn resolved_environments_for_configuration(
|
||||
|
||||
Reference in New Issue
Block a user