From 10d5a603aecbd73a38f3a6576cce69a78f8d6f1d Mon Sep 17 00:00:00 2001 From: jif Date: Wed, 26 Aug 2026 13:43:53 +0000 Subject: [PATCH] Persist Guardian V2 risk scores without restoring them (#40884) ## What changed - Append accepted Guardian V2 classification results to rollout history for non-ephemeral threads. - Keep resumed and forked threads from restoring a persisted score into active Guardian state. ## Testing - Verify that asynchronous scoring records the resulting `SecurityRiskScore` rollout item. - Seed resume and fork tests with a persisted score and verify that approvals ignore it. GitOrigin-RevId: 40d63a2ea7e7cf6398402c6aed7d5e5727dc9d68 --- codex-rs/Cargo.lock | 1 + .../app-server/tests/suite/v2/guardian_v2.rs | 42 ++++++++++++++----- codex-rs/core/src/session/session.rs | 13 ------ codex-rs/ext/guardian-v2/Cargo.toml | 1 + .../guardian-v2/src/async_scorer/extension.rs | 14 +++++++ .../src/async_scorer/extension_tests.rs | 22 ++++++++++ 6 files changed, 70 insertions(+), 23 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 2d3ebb535a..e0a49eb1ab 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3329,6 +3329,7 @@ dependencies = [ "codex-core", "codex-extension-api", "codex-features", + "codex-history", "codex-http-client", "codex-login", "codex-model-provider", diff --git a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs index bc66fda917..3d27bc98bb 100644 --- a/codex-rs/app-server/tests/suite/v2/guardian_v2.rs +++ b/codex-rs/app-server/tests/suite/v2/guardian_v2.rs @@ -1,3 +1,4 @@ +use std::io::Write; use std::sync::Arc; use std::sync::Mutex; use std::sync::atomic::AtomicUsize; @@ -8,6 +9,7 @@ use anyhow::Result; use app_test_support::MockResponsesConfig; use app_test_support::TestAppServer; use app_test_support::create_fake_rollout; +use app_test_support::rollout_path; use axum::Json; use axum::Router; use axum::extract::State; @@ -571,14 +573,34 @@ async fn guardian_v2_routes_scoped_tool_approvals( | ThreadLifecycle::RootUserRestriction | ThreadLifecycle::RootUserInputRestriction | ThreadLifecycle::RootUserInputHookBlocked => None, - ThreadLifecycle::Resume | ThreadLifecycle::Fork => Some(create_fake_rollout( - codex_home.path(), - "2025-01-05T12-00-00", - "2025-01-05T12:00:00Z", - USER_CONTEXT, - Some("mock_provider"), - /*git_info*/ None, - )?), + ThreadLifecycle::Resume | ThreadLifecycle::Fork => { + let thread_id = create_fake_rollout( + codex_home.path(), + "2025-01-05T12-00-00", + "2025-01-05T12:00:00Z", + USER_CONTEXT, + Some("mock_provider"), + /*git_info*/ None, + )?; + let mut rollout = std::fs::OpenOptions::new().append(true).open(rollout_path( + codex_home.path(), + "2025-01-05T12-00-00", + &thread_id, + ))?; + writeln!( + rollout, + "{}", + json!({ + "timestamp": "2025-01-05T12:00:00Z", + "type": "security_risk_score", + "payload": { + "scores": { "action_risk": 0.0 }, + "sampled_at": "2025-01-05T12:00:00Z", + }, + }) + )?; + Some(thread_id) + } }; let mut app_server = TestAppServer::builder() .with_codex_home(codex_home.path()) @@ -1143,7 +1165,7 @@ async fn guardian_v2_required_model_bypasses_scoring_and_runs_full_reviews() -> } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn resumed_thread_starts_without_guardian_score() -> Result<()> { +async fn resumed_thread_ignores_persisted_guardian_score() -> Result<()> { skip_if_no_network!(Ok(())); guardian_v2_routes_tool_approvals( GuardianRisk::Low, @@ -1156,7 +1178,7 @@ async fn resumed_thread_starts_without_guardian_score() -> Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn forked_thread_starts_without_guardian_score() -> Result<()> { +async fn forked_thread_ignores_persisted_guardian_score() -> Result<()> { skip_if_no_network!(Ok(())); guardian_v2_routes_tool_approvals( GuardianRisk::Low, diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index d58f793818..71ad2afef0 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -31,7 +31,6 @@ use codex_protocol::protocol::MultiAgentVersion; use codex_protocol::protocol::ThreadHistoryMode; use codex_protocol::protocol::ThreadSource; use codex_protocol::protocol::TurnEnvironmentSelections; -use codex_protocol::security_risk::SecurityRiskScore; use codex_skills::SkillError; use std::sync::OnceLock; use tokio::sync::Semaphore; @@ -752,18 +751,6 @@ impl Session { config.current_time_reminder.as_ref(), external_time_provider, )?; - if thread_extension_init.get::().is_none() - && let Some(score) = initial_history - .get_rollout_items() - .iter() - .rev() - .find_map(|item| match item { - RolloutItem::SecurityRiskScore(score) => Some(score), - _ => None, - }) - { - thread_extension_init.insert(score.clone()); - } let selected_capability_roots = match thread_extension_init.get::>() { Some(roots) => roots.as_ref().clone(), diff --git a/codex-rs/ext/guardian-v2/Cargo.toml b/codex-rs/ext/guardian-v2/Cargo.toml index e5119b80c0..bf93301bb8 100644 --- a/codex-rs/ext/guardian-v2/Cargo.toml +++ b/codex-rs/ext/guardian-v2/Cargo.toml @@ -17,6 +17,7 @@ codex-api = { workspace = true } codex-core = { workspace = true } codex-extension-api = { workspace = true } codex-features = { workspace = true } +codex-history = { workspace = true } codex-http-client = { workspace = true } codex-login = { workspace = true } codex-model-provider = { workspace = true } diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs index b2ea0d7759..68a9e8cd09 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension.rs @@ -29,6 +29,7 @@ use codex_extension_api::ToolName; use codex_extension_api::ToolPayload; use codex_extension_api::ToolStartInput; use codex_features::Feature; +use codex_history::RolloutItem; use codex_login::AgentIdentityAuthPolicy; use codex_login::AuthManager; use codex_model_provider::create_model_provider; @@ -796,6 +797,19 @@ impl GuardianV2Extension { .latest_scored_tool_call .fetch_max(tool_call_index, Ordering::Release); classification_finished_at = Some(Instant::now()); + if !config.ephemeral + && let Err(error) = thread + .append_rollout_items(&[RolloutItem::SecurityRiskScore(score)]) + .await + { + tracing::warn!( + %thread_id, + %turn_id, + %call_id, + %error, + "failed to persist Guardian V2 classification result" + ); + } Ok("success") } .await; diff --git a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs index 61f455b5b4..f486ddc016 100644 --- a/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs +++ b/codex-rs/ext/guardian-v2/src/async_scorer/extension_tests.rs @@ -22,6 +22,7 @@ use codex_extension_api::ToolName; use codex_extension_api::ToolPayload; use codex_extension_api::ToolStartInput; use codex_features::Feature; +use codex_history::RolloutItem; use codex_login::AuthManager; use codex_login::CodexAuth; use codex_login::ExternalAuth; @@ -1726,6 +1727,27 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result< } ); assert!(score.sampled_at.is_some()); + test.codex.ensure_rollout_materialized().await; + let persisted_score = tokio::time::timeout(Duration::from_secs(5), async { + loop { + if let Some(persisted_score) = test + .codex + .load_history(/*include_archived*/ false) + .await? + .items + .into_iter() + .find_map(|item| match item { + RolloutItem::SecurityRiskScore(score) => Some(score), + _ => None, + }) + { + return Ok::<_, anyhow::Error>(persisted_score); + } + tokio::task::yield_now().await; + } + }) + .await??; + assert_eq!(&persisted_score, score.as_ref()); assert_eq!( registry .fast_approval_decision(