mirror of
https://github.com/openai/codex.git
synced 2026-10-21 09:54:56 +00:00
Skip Guardian scoring in User approval mode (#42256)
## What changed - Skip Guardian prewarming and asynchronous scoring when `approvalsReviewer` is `"user"`, including when the reviewer changes during an active turn. - Automatically accept ordinary `node_repl.js` execution confirmations in this mode while continuing to surface sensitive-action checks and requests for user input. - Cover transitions between User approval, automatic review, and Full Access in the app-server integration tests. GitOrigin-RevId: 599c8ebb672728b9bf3d64c34377ddec230325f8
This commit is contained in:
@@ -1923,6 +1923,11 @@ persisted rollout errors, so unavailable details after a restart remain a termin
|
||||
|
||||
## Approvals
|
||||
|
||||
In User approval mode (`approvalsReviewer: "user"`), async Guardian scoring and
|
||||
prewarming are skipped, and ordinary `node_repl.js` execution confirmations are
|
||||
accepted automatically. Separate sensitive-action checks and requests for user
|
||||
input keep their existing behavior. Approve for me and Full Access are unchanged.
|
||||
|
||||
Full Access (`approvalPolicy: "never"` with unrestricted selected environments)
|
||||
skips Guardian, including background scoring. Confirmation-only MCP approvals,
|
||||
including strict or sensitive CUA requests, are accepted. Strict responses retain
|
||||
|
||||
@@ -23,8 +23,10 @@ use axum::routing::get;
|
||||
use axum::routing::post;
|
||||
use codex_app_server_protocol::ApprovalsReviewer;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::ItemCompletedNotification;
|
||||
use codex_app_server_protocol::ItemGuardianApprovalReviewStartedNotification;
|
||||
use codex_app_server_protocol::McpServerElicitationRequest;
|
||||
use codex_app_server_protocol::McpToolCallStatus;
|
||||
use codex_app_server_protocol::SandboxMode;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
@@ -39,6 +41,9 @@ use codex_app_server_protocol::ThreadRollbackParams;
|
||||
use codex_app_server_protocol::ThreadRollbackResponse;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_app_server_protocol::TurnCompletedNotification;
|
||||
use codex_app_server_protocol::TurnSettingsUpdateParams;
|
||||
use codex_app_server_protocol::TurnSettingsUpdateResponse;
|
||||
use codex_app_server_protocol::TurnSettingsUpdateStatus;
|
||||
use codex_app_server_protocol::TurnStartParams;
|
||||
use codex_app_server_protocol::TurnStartResponse;
|
||||
use codex_app_server_protocol::TurnStatus;
|
||||
@@ -161,6 +166,7 @@ struct MockResponsesState {
|
||||
transcript_content: TranscriptContent,
|
||||
mcp_server_name: Option<&'static str>,
|
||||
mcp_tool_sequence: Option<&'static [&'static str]>,
|
||||
mcp_messages: Mutex<Vec<&'static str>>,
|
||||
root_worker: bool,
|
||||
root_user_restriction: bool,
|
||||
root_user_input_restriction: bool,
|
||||
@@ -448,7 +454,13 @@ async fn parent_response(
|
||||
|| (state.root_worker || state.root_user_restriction) && request_number == 3
|
||||
{
|
||||
let call_id = format!("guardian-action-{request_number}");
|
||||
let mut message = format!("guardian-{request_number}");
|
||||
let mut message = state
|
||||
.mcp_messages
|
||||
.lock()
|
||||
.expect("MCP messages lock")
|
||||
.get(request_number)
|
||||
.map(|message| (*message).to_string())
|
||||
.unwrap_or_else(|| format!("guardian-{request_number}"));
|
||||
if request_number == 0 && matches!(state.review_outcome, ReviewOutcome::Deny) {
|
||||
message.push_str(&"x".repeat(2_000));
|
||||
}
|
||||
@@ -1817,11 +1829,11 @@ async fn first_cua_review_does_not_wait_for_initial_score(
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> {
|
||||
async fn user_approval_skips_async_guardian_without_changing_other_modes() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
let state = Arc::new(MockResponsesState {
|
||||
mcp_server_name: Some("node_repl"),
|
||||
mcp_tool_sequence: Some(&["js", "js"]),
|
||||
mcp_tool_sequence: Some(&["js", "js", "js"]),
|
||||
..Default::default()
|
||||
});
|
||||
let listener = TcpListener::bind("127.0.0.1:0").await?;
|
||||
@@ -1832,15 +1844,15 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> {
|
||||
let responses_server = tokio::spawn(async move {
|
||||
let _ = axum::serve(listener, router).await;
|
||||
});
|
||||
// Exercise strict, sensitive CUA confirmations using the existing MCP server.
|
||||
// Mix ordinary execution, a user-input form, and a sensitive check.
|
||||
let (mcp_url, mcp_server) =
|
||||
start_mcp_server_with_tools(&["js"], /*sensitive_action*/ Some(true)).await?;
|
||||
start_mcp_server_with_tools(&["js"], /*sensitive_action*/ None).await?;
|
||||
let codex_home = TempDir::new()?;
|
||||
MockResponsesConfig::new(&responses_url)
|
||||
.with_model(MODEL)
|
||||
.with_provider_config("supports_websockets = false")
|
||||
.with_approval_policy("never")
|
||||
.with_root_config("approvals_reviewer = \"auto_review\"")
|
||||
.with_approval_policy("on-request")
|
||||
.with_root_config("approvals_reviewer = \"user\"")
|
||||
.with_extra_config(&format!(
|
||||
"[mcp_servers.node_repl]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[features.guardianv2]\nenabled = true"
|
||||
))
|
||||
@@ -1856,7 +1868,8 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> {
|
||||
.await?;
|
||||
let thread = app_server
|
||||
.start_thread(ThreadStartParams {
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
approval_policy: Some(AskForApproval::OnRequest),
|
||||
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||
sandbox: Some(SandboxMode::DangerFullAccess),
|
||||
..Default::default()
|
||||
})
|
||||
@@ -1865,26 +1878,82 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> {
|
||||
|
||||
// Leave async scoring pending so reviewed mode must use synchronous Guardian.
|
||||
state.allow_guardian_review.notify_one();
|
||||
for approval_policy in [AskForApproval::Never, AskForApproval::OnRequest] {
|
||||
use ApprovalsReviewer::AutoReview;
|
||||
use ApprovalsReviewer::User;
|
||||
use AskForApproval::Never;
|
||||
use AskForApproval::OnRequest;
|
||||
for (approval_policy, reviewer, live_reviewer, expected_reviews) in [
|
||||
(OnRequest, User, None, 1),
|
||||
(Never, User, None, 0),
|
||||
(Never, AutoReview, None, 0),
|
||||
(OnRequest, User, Some(AutoReview), 2),
|
||||
(OnRequest, AutoReview, Some(User), 2),
|
||||
(OnRequest, AutoReview, None, 3),
|
||||
(OnRequest, User, None, 1),
|
||||
] {
|
||||
let reviews_before = state.guardian_reviews.load(Ordering::SeqCst);
|
||||
*state.mcp_messages.lock().expect("MCP messages lock") = vec![
|
||||
if approval_policy == OnRequest {
|
||||
"confirm"
|
||||
} else {
|
||||
"guardian-0"
|
||||
},
|
||||
"guardian-1",
|
||||
"sensitive",
|
||||
];
|
||||
state.parent_requests.store(0, Ordering::SeqCst);
|
||||
app_server.clear_message_buffer();
|
||||
let completed = timeout(
|
||||
TIMEOUT,
|
||||
app_server.start_turn_and_wait_for_completion(TurnStartParams {
|
||||
let request_id = app_server
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread.id.clone(),
|
||||
input: vec![UserInput::Text {
|
||||
text: USER_CONTEXT.to_owned(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
approval_policy: Some(approval_policy),
|
||||
approvals_reviewer: Some(reviewer),
|
||||
..Default::default()
|
||||
}),
|
||||
)
|
||||
.await??;
|
||||
})
|
||||
.await?;
|
||||
let started: TurnStartResponse =
|
||||
timeout(TIMEOUT, app_server.read_response(request_id)).await??;
|
||||
if approval_policy == OnRequest {
|
||||
let request =
|
||||
timeout(TIMEOUT, app_server.read_stream_until_request_message()).await??;
|
||||
let ServerRequest::McpServerElicitationRequest { request_id, params } = request else {
|
||||
panic!("expected user input, got {request:?}");
|
||||
};
|
||||
assert!(
|
||||
matches!(params.request, McpServerElicitationRequest::Form { message, .. }
|
||||
if message == "Allow this request?")
|
||||
);
|
||||
if let Some(reviewer) = live_reviewer {
|
||||
let updated: TurnSettingsUpdateResponse = app_server
|
||||
.request(|request_id| ClientRequest::TurnSettingsUpdate {
|
||||
request_id,
|
||||
params: TurnSettingsUpdateParams {
|
||||
thread_id: thread.id.clone(),
|
||||
turn_id: started.turn.id.clone(),
|
||||
approvals_reviewer: Some(reviewer),
|
||||
..Default::default()
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(updated.status, TurnSettingsUpdateStatus::Applied);
|
||||
}
|
||||
app_server
|
||||
.send_response(
|
||||
request_id,
|
||||
json!({ "action": "accept", "content": { "confirmed": true } }),
|
||||
)
|
||||
.await?;
|
||||
}
|
||||
let completed: TurnCompletedNotification =
|
||||
timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??;
|
||||
assert_eq!(completed.turn.status, TurnStatus::Completed);
|
||||
|
||||
let mut tool_results = Vec::new();
|
||||
while tool_results.len() < 2 {
|
||||
while tool_results.len() < 3 {
|
||||
let completed: ItemCompletedNotification =
|
||||
timeout(TIMEOUT, app_server.read_notification("item/completed")).await??;
|
||||
if let ThreadItem::McpToolCall {
|
||||
@@ -1899,22 +1968,28 @@ async fn full_access_skips_guardian_until_review_is_enabled() -> Result<()> {
|
||||
}
|
||||
assert_eq!(
|
||||
tool_results,
|
||||
(0..2)
|
||||
(0..3)
|
||||
.map(|index| (
|
||||
McpToolCallStatus::Completed,
|
||||
Some(vec![
|
||||
json!({ "type": "text", "text": format!("echo: guardian-{index}") })
|
||||
]),
|
||||
Some(vec![json!({ "type": "text", "text": match index {
|
||||
0 if approval_policy == OnRequest => "accepted".to_string(),
|
||||
2 => "echo: sensitive".to_string(),
|
||||
_ => format!("echo: guardian-{index}"),
|
||||
} })]),
|
||||
None,
|
||||
))
|
||||
.collect::<Vec<_>>()
|
||||
);
|
||||
if approval_policy == AskForApproval::Never {
|
||||
assert_eq!(state.guardian_reviews.load(Ordering::SeqCst), 0);
|
||||
assert_eq!(state.luna_connections.load(Ordering::SeqCst), 0);
|
||||
} else {
|
||||
assert_eq!(
|
||||
state.guardian_reviews.load(Ordering::SeqCst),
|
||||
reviews_before + expected_reviews
|
||||
);
|
||||
if approval_policy == OnRequest
|
||||
&& (reviewer == AutoReview || live_reviewer == Some(AutoReview))
|
||||
{
|
||||
wait_for_luna_request(&state, /*index*/ 0).await?;
|
||||
assert_eq!(state.guardian_reviews.load(Ordering::SeqCst), 2);
|
||||
} else if reviews_before <= 1 {
|
||||
assert_eq!(state.luna_connections.load(Ordering::SeqCst), 0);
|
||||
}
|
||||
}
|
||||
app_server.shutdown_gracefully().await?;
|
||||
|
||||
@@ -1369,7 +1369,10 @@ impl ServerHandler for ToolAppsMcpServer {
|
||||
// Match the execution approval emitted by the real Node REPL server.
|
||||
approval_meta["connector_id"] = json!("node_repl");
|
||||
}
|
||||
if let Some(sensitive_action) = self.sensitive_action {
|
||||
if let Some(sensitive_action) = self
|
||||
.sensitive_action
|
||||
.or((message == "sensitive").then_some(true))
|
||||
{
|
||||
approval_meta["codex_sensitive_action"] = json!(sensitive_action);
|
||||
}
|
||||
let result = context
|
||||
|
||||
@@ -733,6 +733,20 @@ impl CodexThread {
|
||||
self.session.thread_config_snapshot().await
|
||||
}
|
||||
|
||||
/// Returns the active turn's reviewer, including live updates, or the thread default.
|
||||
pub async fn approvals_reviewer_for_turn(&self, turn_id: &str) -> ApprovalsReviewer {
|
||||
if let Some((turn, settings, _)) = self
|
||||
.session
|
||||
.active_turn_context_and_strict_auto_review()
|
||||
.await
|
||||
&& turn.sub_id == turn_id
|
||||
{
|
||||
settings.approvals_reviewer()
|
||||
} else {
|
||||
self.config_snapshot().await.approvals_reviewer
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns thread-owned settings suitable for rollout persistence and resume.
|
||||
pub async fn thread_settings_snapshot(&self) -> ThreadSettingsSnapshot {
|
||||
self.session.thread_settings_snapshot().await
|
||||
|
||||
@@ -726,19 +726,34 @@ async fn review_guardian_mcp_elicitation(
|
||||
};
|
||||
let step_settings = turn_context.current_settings.load_full();
|
||||
|
||||
// User approval skips ordinary CUA checks, not separate sensitive requests.
|
||||
let user_cua_execution = step_settings.approvals_reviewer() == ApprovalsReviewer::User
|
||||
&& is_node_repl_backed_server(&request.server_name)
|
||||
&& request.elicitation.meta().is_some_and(|meta| {
|
||||
metadata_str(meta, MCP_ELICITATION_TOOL_NAME_KEY) == Some("js")
|
||||
&& metadata_str(meta, MCP_ELICITATION_CONNECTOR_ID_KEY) == Some("node_repl")
|
||||
&& metadata_str(meta, MCP_ELICITATION_APPROVAL_KIND_KEY)
|
||||
== Some(MCP_ELICITATION_APPROVAL_KIND_MCP_TOOL_CALL)
|
||||
&& meta.get(MCP_ELICITATION_SENSITIVE_ACTION_KEY) != Some(&Value::Bool(true))
|
||||
&& meta.get("codex_requires_user_input") != Some(&Value::Bool(true))
|
||||
});
|
||||
|
||||
// Full Access skips inference, not the active-turn and cancellation checks.
|
||||
if turn_context.environments.has_full_access(
|
||||
turn_context.approval_policy(),
|
||||
&turn_context
|
||||
.config
|
||||
.permissions
|
||||
.effective_permission_profile(),
|
||||
) && matches!(
|
||||
&request.elicitation,
|
||||
Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams {
|
||||
requested_schema, ..
|
||||
}) if requested_schema.properties.is_empty()
|
||||
) {
|
||||
if (user_cua_execution
|
||||
|| turn_context.environments.has_full_access(
|
||||
turn_context.approval_policy(),
|
||||
&turn_context
|
||||
.config
|
||||
.permissions
|
||||
.effective_permission_profile(),
|
||||
))
|
||||
&& matches!(
|
||||
&request.elicitation,
|
||||
Elicitation::Mcp(rmcp::model::ElicitRequestParams::FormElicitationParams {
|
||||
requested_schema, ..
|
||||
}) if requested_schema.properties.is_empty()
|
||||
)
|
||||
{
|
||||
let decision = if cancellation_token.is_cancelled() {
|
||||
ReviewDecision::Abort
|
||||
} else {
|
||||
|
||||
@@ -40,6 +40,7 @@ use codex_login::AgentIdentityAuthPolicy;
|
||||
use codex_login::AuthManager;
|
||||
use codex_model_provider::create_model_provider;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::mcp::is_node_repl_backed_server;
|
||||
use codex_protocol::mcp::is_node_repl_backed_tool;
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
@@ -214,16 +215,18 @@ impl ThreadLifecycleContributor<Config> for GuardianV2Extension {
|
||||
.insert(TrustedSkillRoots::from_config(input.config));
|
||||
input.thread_store.insert(guardian_v2_enabled);
|
||||
|
||||
// Keep the sampler available if a later turn leaves Full Access, but do
|
||||
// not open Guardian connections while Full Access is selected.
|
||||
if !has_full_access(
|
||||
input.config.permissions.approval_policy.value(),
|
||||
&input.config.permissions.effective_permission_profile(),
|
||||
input
|
||||
.environments
|
||||
.iter()
|
||||
.map(|environment| &environment.config),
|
||||
) {
|
||||
// Keep the sampler available for later automatic review, but do not
|
||||
// prewarm while User approval mode or Full Access is selected.
|
||||
if input.config.approvals_reviewer == ApprovalsReviewer::AutoReview
|
||||
&& !has_full_access(
|
||||
input.config.permissions.approval_policy.value(),
|
||||
&input.config.permissions.effective_permission_profile(),
|
||||
input
|
||||
.environments
|
||||
.iter()
|
||||
.map(|environment| &environment.config),
|
||||
)
|
||||
{
|
||||
tokio::spawn(async move {
|
||||
sampler.prewarm().await;
|
||||
});
|
||||
@@ -493,10 +496,11 @@ impl GuardianV2Extension {
|
||||
return;
|
||||
}
|
||||
};
|
||||
// Read current permissions, not the startup config: existing threads can
|
||||
// enter Full Access after the sampler has already been initialized.
|
||||
// Use the live reviewer, not the startup config or per-app reviewer overrides.
|
||||
let snapshot = thread.config_snapshot().await;
|
||||
if snapshot.full_access {
|
||||
if snapshot.full_access
|
||||
|| thread.approvals_reviewer_for_turn(input.turn_id).await == ApprovalsReviewer::User
|
||||
{
|
||||
// A skipped call invalidates older scores, including ones still in flight.
|
||||
score_progress
|
||||
.latest_failed_tool_call
|
||||
|
||||
@@ -34,6 +34,7 @@ use codex_login::ExternalAuthRefreshContext;
|
||||
use codex_model_provider_info::ModelProviderInfo;
|
||||
use codex_protocol::ResponseItemId;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputContentItem;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
@@ -115,7 +116,10 @@ async fn installed_extension_warms_connections_without_blocking_thread_start() -
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let thread_server = responses::start_mock_server().await;
|
||||
let test = test_codex().build_with_auto_env(&thread_server).await?;
|
||||
let test = test_codex()
|
||||
.with_config(|config| config.approvals_reviewer = ApprovalsReviewer::AutoReview)
|
||||
.build_with_auto_env(&thread_server)
|
||||
.await?;
|
||||
let mut connections = vec![
|
||||
WebSocketConnectionConfig {
|
||||
requests: Vec::new(),
|
||||
@@ -172,7 +176,10 @@ async fn installed_extension_reconnects_after_auth_refresh() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let thread_server = responses::start_mock_server().await;
|
||||
let test = test_codex().build_with_auto_env(&thread_server).await?;
|
||||
let test = test_codex()
|
||||
.with_config(|config| config.approvals_reviewer = ApprovalsReviewer::AutoReview)
|
||||
.build_with_auto_env(&thread_server)
|
||||
.await?;
|
||||
let events = vec![
|
||||
ev_assistant_message("sample", "low"),
|
||||
ev_completed("response-1"),
|
||||
@@ -903,7 +910,10 @@ async fn sample_configured_conversation_history_with_source(
|
||||
.policy = Some(TEST_CATALOG_GUARDIAN_POLICY.to_owned());
|
||||
})
|
||||
.with_model("gpt-5.5")
|
||||
.with_config(move |config| config.guardian_policy_config = guardian_policy)
|
||||
.with_config(move |config| {
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config.guardian_policy_config = guardian_policy;
|
||||
})
|
||||
.with_pre_build_hook(move |home| {
|
||||
std::fs::write(home.join("config.toml"), guardian_config)
|
||||
.expect("Guardian v2 configuration should be written");
|
||||
@@ -2171,6 +2181,7 @@ async fn contributor_skips_required_models_in_standard_scope() -> Result<()> {
|
||||
let test = test_codex()
|
||||
.with_home(Arc::clone(&initial.home))
|
||||
.with_config(move |config| {
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config.config_layer_stack = config_layer_stack;
|
||||
config
|
||||
.features
|
||||
@@ -2806,6 +2817,7 @@ async fn contributor_reuses_the_latest_compatible_parent_compaction() -> Result<
|
||||
|
||||
let thread_server = responses::start_mock_server().await;
|
||||
let test = test_codex()
|
||||
.with_config(|config| config.approvals_reviewer = ApprovalsReviewer::AutoReview)
|
||||
.with_pre_build_hook(|home| {
|
||||
std::fs::write(
|
||||
home.join("config.toml"),
|
||||
|
||||
Reference in New Issue
Block a user