mirror of
https://github.com/openai/codex.git
synced 2026-10-22 17:58:46 +00:00
Honor required reviews when reusing Guardian scores (#41309)
## Why After switching a thread to a model that requires automatic review, a standard-scope approval could reuse a cached low-risk score for an action that skipped classification. This allowed the action to bypass the required full review. ## What changed Check the thread's current model requirements before making a fast Guardian decision. Defer required models to the full review path instead of approving from cached scoring state. ## Testing Add coverage for switching to a required model and reviewing a sandboxed command without running another classification. GitOrigin-RevId: e79e6a5df19c46acd5c4176f522a859793177639
This commit is contained in:
@@ -62,6 +62,7 @@ use super::mcp_tool::start_mcp_server;
|
||||
|
||||
const TIMEOUT: Duration = Duration::from_secs(30);
|
||||
const MODEL: &str = "mock-model";
|
||||
const REQUIRED_MODEL: &str = "protected-model";
|
||||
const USER_CONTEXT: &str = "The user authorized reading the existing project files.";
|
||||
const ROOT_RESTRICTION: &str =
|
||||
"I revoke authorization for the MCP tool. Tell the worker to reassess its previous action.";
|
||||
@@ -191,6 +192,7 @@ enum GuardianToolScope {
|
||||
#[derive(Clone, Copy)]
|
||||
enum ThreadLifecycle {
|
||||
New,
|
||||
RequiredModelSwitch,
|
||||
UserInputRestriction,
|
||||
UserInputEmpty,
|
||||
UserInputHookFeedback,
|
||||
@@ -415,7 +417,17 @@ async fn parent_response(
|
||||
.contains("Completed synchronous Guardian review.")
|
||||
);
|
||||
let request_number = state.parent_requests.fetch_add(1, Ordering::SeqCst);
|
||||
if state.user_input_restriction && request_number == 1 {
|
||||
if request["model"] == REQUIRED_MODEL && request_number == 3 {
|
||||
vec![
|
||||
responses::ev_response_created("required-model-command"),
|
||||
responses::ev_function_call(
|
||||
"required-model-command",
|
||||
"exec_command",
|
||||
r#"{"cmd":"echo required-model","login":false}"#,
|
||||
),
|
||||
responses::ev_completed("required-model-command"),
|
||||
]
|
||||
} else if state.user_input_restriction && request_number == 1 {
|
||||
user_input_request_events()
|
||||
} else if request_number < 2
|
||||
|| state.user_input_restriction && request_number == 2
|
||||
@@ -639,6 +651,18 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
),
|
||||
)?;
|
||||
}
|
||||
if matches!(lifecycle, ThreadLifecycle::RequiredModelSwitch) {
|
||||
let rules_dir = codex_home.path().join("rules");
|
||||
std::fs::create_dir_all(&rules_dir)?;
|
||||
std::fs::write(
|
||||
rules_dir.join("default.rules"),
|
||||
r#"prefix_rule(pattern=["echo"], decision="prompt")"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
codex_home.path().join("requirements.toml"),
|
||||
format!("[auto_review]\nrequired_on_models = [\"{REQUIRED_MODEL}\"]\n"),
|
||||
)?;
|
||||
}
|
||||
let (reviewer_config, requested_reviewer) = match requirement {
|
||||
ModelReviewRequirement::Optional => (
|
||||
"approvals_reviewer = \"auto_review\"",
|
||||
@@ -689,6 +713,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
}
|
||||
let original_thread_id = match lifecycle {
|
||||
ThreadLifecycle::New
|
||||
| ThreadLifecycle::RequiredModelSwitch
|
||||
| ThreadLifecycle::UserInputRestriction
|
||||
| ThreadLifecycle::UserInputEmpty
|
||||
| ThreadLifecycle::UserInputHookFeedback
|
||||
@@ -736,6 +761,7 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
.await?;
|
||||
let thread = match lifecycle {
|
||||
ThreadLifecycle::New
|
||||
| ThreadLifecycle::RequiredModelSwitch
|
||||
| ThreadLifecycle::UserInputRestriction
|
||||
| ThreadLifecycle::UserInputEmpty
|
||||
| ThreadLifecycle::UserInputHookFeedback
|
||||
@@ -1162,6 +1188,45 @@ async fn guardian_v2_routes_scoped_tool_approvals(
|
||||
);
|
||||
}
|
||||
|
||||
if matches!(lifecycle, ThreadLifecycle::RequiredModelSwitch) {
|
||||
// Both MCP actions have low scores; the sandboxed exec must still receive full review.
|
||||
timeout(TIMEOUT, responses_state.classification_completed.notified()).await?;
|
||||
let request_id = app_server
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread_id.clone(),
|
||||
model: Some(REQUIRED_MODEL.to_owned()),
|
||||
input: vec![UserInput::Text {
|
||||
text: "Run the requested command.".to_owned(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(request_id)).await??;
|
||||
let review_started: ItemGuardianApprovalReviewStartedNotification = timeout(
|
||||
TIMEOUT,
|
||||
app_server.read_notification("item/autoApprovalReview/started"),
|
||||
)
|
||||
.await??;
|
||||
assert_eq!(review_started.thread_id, thread_id);
|
||||
let completed: TurnCompletedNotification =
|
||||
timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??;
|
||||
assert_eq!(completed.thread_id, thread_id);
|
||||
assert_eq!(
|
||||
responses_state.guardian_reviews.load(Ordering::SeqCst),
|
||||
expected_guardian_reviews + 1,
|
||||
);
|
||||
assert_eq!(
|
||||
responses_state
|
||||
.luna_requests
|
||||
.lock()
|
||||
.expect("Luna request lock should not be poisoned")
|
||||
.len(),
|
||||
2,
|
||||
"the sandboxed command must skip classification",
|
||||
);
|
||||
}
|
||||
|
||||
if !late_root_restriction
|
||||
&& (lifecycle.uses_root_worker()
|
||||
|| matches!(lifecycle, ThreadLifecycle::RootUserRestriction))
|
||||
@@ -1682,6 +1747,23 @@ async fn guardian_v2_required_model_bypasses_scoring_and_runs_full_reviews() ->
|
||||
.await
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn guardian_v2_required_model_cannot_reuse_a_cached_score_for_skipped_exec() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_wine_exec!(
|
||||
Ok(()),
|
||||
"the echo prompt rule requires host-native shell command parsing"
|
||||
);
|
||||
guardian_v2_routes_tool_approvals(
|
||||
GuardianRisk::Low,
|
||||
ThreadLifecycle::RequiredModelSwitch,
|
||||
ModelReviewRequirement::Optional,
|
||||
ReviewOutcome::Allow,
|
||||
TranscriptContent::Normal,
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn resumed_thread_ignores_persisted_guardian_score() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -261,6 +261,24 @@ impl ApprovalReviewContributor for GuardianV2Extension {
|
||||
record_fast_decision(extension_metrics.as_deref(), "deferred", "out_of_scope");
|
||||
return None;
|
||||
}
|
||||
} else if thread_store.get::<ModelInfo>().is_some() {
|
||||
let manager = self.thread_manager.upgrade()?;
|
||||
let thread_id = ThreadId::from_string(thread_store.level_id()).ok()?;
|
||||
let thread = manager.get_thread(thread_id).await.ok()?;
|
||||
let config = thread.config().await;
|
||||
let model = thread_store.get::<ModelInfo>()?;
|
||||
if config
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.auto_review_required_for_model(&model.slug)
|
||||
{
|
||||
record_fast_decision(
|
||||
extension_metrics.as_deref(),
|
||||
"deferred",
|
||||
"required_model",
|
||||
);
|
||||
return None;
|
||||
}
|
||||
}
|
||||
let Some(score_progress) = thread_store.get::<GuardianV2ScoreProgress>() else {
|
||||
record_fast_decision(extension_metrics.as_deref(), "deferred", "missing_score");
|
||||
|
||||
@@ -2180,7 +2180,7 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> {
|
||||
/*extension_metrics*/ None,
|
||||
)
|
||||
.await,
|
||||
Some(ReviewDecision::Approved)
|
||||
None
|
||||
);
|
||||
|
||||
let turn_store = ExtensionData::new("turn-1");
|
||||
|
||||
Reference in New Issue
Block a user