mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Let extensions resolve approval reviews before Guardian (#38592)
## What changed - Give approval review contributors the first opportunity to resolve an action when automatic review is not required for the model. - Return an extension's decision directly and fall back to Guardian when no extension claims the review. - Update Guardian V2 to approve actions below its risk threshold through the review contributor API, and remove the extension approval-policy API. ## Testing - Verify that an extension approval bypasses both the Guardian model call and the user approval prompt. - Cover Guardian V2 review decisions for low-risk, high-risk, and disabled threads. GitOrigin-RevId: 5be56941143aff6f358b75b5f8101b7c047147ae
This commit is contained in:
1 parent
5bc8da6d78
commit
23094236ac
12 files changed
+174
-149
No files matched your search
@@ -4,8 +4,7 @@ use std::time::SystemTime;
|
||||
|
||||
use codex_core::ThreadManager;
|
||||
use codex_core::config::Config;
|
||||
use codex_extension_api::ApprovalPolicyContributor;
|
||||
use codex_extension_api::ApprovalRequirement;
|
||||
use codex_extension_api::ApprovalReviewContributor;
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionEventSink;
|
||||
use codex_extension_api::ExtensionFuture;
|
||||
@@ -28,6 +27,7 @@ use codex_model_provider::create_model_provider;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::TruncationPolicy;
|
||||
use codex_protocol::security_risk::SecurityRiskScore;
|
||||
use serde_json::json;
|
||||
@@ -198,7 +198,9 @@ impl ThreadLifecycleContributor<Config> for GuardianV2Extension {
|
||||
input: ThreadStartInput<'a, Config>,
|
||||
) -> ExtensionFuture<'a, ()> {
|
||||
Box::pin(async move {
|
||||
if !input.config.features.enabled(Feature::GuardianV2) {
|
||||
if !input.config.features.enabled(Feature::GuardianV2)
|
||||
|| !input.config.features.enabled(Feature::GuardianApproval)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -250,23 +252,22 @@ impl ThreadLifecycleContributor<Config> for GuardianV2Extension {
|
||||
}
|
||||
}
|
||||
|
||||
impl ApprovalPolicyContributor for GuardianV2Extension {
|
||||
fn approval_requirement(&self, thread_store: &ExtensionData) -> ApprovalRequirement {
|
||||
if thread_store.get::<GuardianV2Enabled>().is_none() {
|
||||
return ApprovalRequirement::Default;
|
||||
}
|
||||
impl ApprovalReviewContributor for GuardianV2Extension {
|
||||
fn contribute<'a>(
|
||||
&'a self,
|
||||
_session_store: &'a ExtensionData,
|
||||
thread_store: &'a ExtensionData,
|
||||
_prompt: &'a str,
|
||||
) -> ExtensionFuture<'a, Option<ReviewDecision>> {
|
||||
Box::pin(async move {
|
||||
thread_store.get::<GuardianV2Enabled>()?;
|
||||
|
||||
match thread_store.get::<SecurityRiskScore>() {
|
||||
Some(score)
|
||||
if score
|
||||
.scores
|
||||
.get("action_risk")
|
||||
.is_some_and(|score| *score >= ACTION_RISK_REVIEW_THRESHOLD) =>
|
||||
{
|
||||
ApprovalRequirement::RequireAutomaticReview
|
||||
}
|
||||
_ => ApprovalRequirement::Default,
|
||||
}
|
||||
thread_store
|
||||
.get::<SecurityRiskScore>()
|
||||
.and_then(|score| score.scores.get("action_risk").copied())
|
||||
.filter(|score| *score < ACTION_RISK_REVIEW_THRESHOLD)
|
||||
.map(|_| ReviewDecision::Approved)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -442,7 +443,7 @@ pub fn install(
|
||||
thread_manager,
|
||||
});
|
||||
registry.thread_lifecycle_contributor(extension.clone());
|
||||
registry.approval_policy_contributor(extension.clone());
|
||||
registry.approval_review_contributor(extension.clone());
|
||||
registry.tool_lifecycle_contributor(extension);
|
||||
}
|
||||
|
||||
|
||||
@@ -4,7 +4,6 @@ use std::time::Duration;
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_core::config::Config;
|
||||
use codex_extension_api::ApprovalRequirement;
|
||||
use codex_extension_api::ConversationHistorySnapshot;
|
||||
use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::ExtensionRegistry;
|
||||
@@ -24,6 +23,7 @@ use codex_protocol::ResponseItemId;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::models::ReasoningItemReasoningSummary;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::TruncationPolicy;
|
||||
use codex_protocol::security_risk::SecurityRiskScore;
|
||||
@@ -143,8 +143,10 @@ async fn sample_conversation_history(
|
||||
let session_store = ExtensionData::new("session-1");
|
||||
let thread_store = test.codex.thread_extension_data();
|
||||
assert_eq!(
|
||||
registry.approval_requirement(thread_store),
|
||||
ApprovalRequirement::Default
|
||||
registry
|
||||
.approval_review(&session_store, thread_store, "review action")
|
||||
.await,
|
||||
None
|
||||
);
|
||||
registry.thread_lifecycle_contributors()[0]
|
||||
.on_thread_start(ThreadStartInput {
|
||||
@@ -238,6 +240,7 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
||||
let (request, test, registry) =
|
||||
sample_conversation_history(conversation_history, r#"{"path":"README.md"}"#).await?;
|
||||
let thread_id = test.session_configured.thread_id;
|
||||
let session_store = ExtensionData::new("session-1");
|
||||
let thread_store = test.codex.thread_extension_data();
|
||||
assert_eq!(request["model"], "gpt-5.6-luna");
|
||||
assert_eq!(
|
||||
@@ -292,8 +295,10 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
||||
);
|
||||
assert!(score.sampled_at.is_some());
|
||||
assert_eq!(
|
||||
registry.approval_requirement(thread_store),
|
||||
ApprovalRequirement::RequireAutomaticReview
|
||||
registry
|
||||
.approval_review(&session_store, thread_store, "review action")
|
||||
.await,
|
||||
None
|
||||
);
|
||||
test.codex.ensure_rollout_materialized().await;
|
||||
test.codex.flush_rollout().await?;
|
||||
@@ -315,18 +320,22 @@ async fn contributor_samples_tool_calls_with_the_existing_luna_pool() -> Result<
|
||||
sampled_at: None,
|
||||
});
|
||||
assert_eq!(
|
||||
registry.approval_requirement(thread_store),
|
||||
ApprovalRequirement::Default
|
||||
registry
|
||||
.approval_review(&session_store, thread_store, "review action")
|
||||
.await,
|
||||
Some(ReviewDecision::Approved)
|
||||
);
|
||||
|
||||
let disabled_thread_store = ExtensionData::new("disabled-thread");
|
||||
disabled_thread_store.insert(SecurityRiskScore {
|
||||
scores: BTreeMap::from([("action_risk".to_string(), 0.8)]),
|
||||
scores: BTreeMap::from([("action_risk".to_string(), 0.25)]),
|
||||
sampled_at: None,
|
||||
});
|
||||
assert_eq!(
|
||||
registry.approval_requirement(&disabled_thread_store),
|
||||
ApprovalRequirement::Default
|
||||
registry
|
||||
.approval_review(&session_store, &disabled_thread_store, "review action")
|
||||
.await,
|
||||
None
|
||||
);
|
||||
|
||||
Ok(())
|
||||
|
||||
Reference in new issue
Block a user