Route MCP elicitations through the shared approval decision path (#43447)

## What changed

- Use `decide_approval` for MCP elicitations, carrying the effective approval policy, reviewer, and synchronous-review requirement into the decision.
- Preserve unsupported form and URL elicitations for user review.
- Use `computer_use_review_required()` for review evidence, session policy, and turn metadata so model policy takes precedence over the legacy flag.
- Derive strict-review notifications from each assessment's review reason.

## Testing

Add coverage for model policy precedence, user and full-access modes, sensitive elicitations, independent code-mode and nested-tool policies, cached score reuse, and user review of unsupported elicitations.

GitOrigin-RevId: d0a9c40572dca7b3dbf876230d2162b72728084d
This commit is contained in:
jif authored and copyberry committed 2026-09-07 12:04:27 +00:00
1 parent c0b6285711
commit ce5c4133bd
12 files changed
+585 -129

No files matched your search

@@ -97,7 +97,6 @@ use codex_app_server_protocol::item_event_to_server_notification;
use codex_core::CodexThread; use codex_core::CodexThread;
use codex_core::ThreadManager; use codex_core::ThreadManager;
use codex_features::Feature; use codex_features::Feature;
use codex_guardian_v2::StrictReviewReason;
use codex_protocol::ThreadId; use codex_protocol::ThreadId;
use codex_protocol::items::CollabAgentTool as CoreCollabAgentTool; use codex_protocol::items::CollabAgentTool as CoreCollabAgentTool;
use codex_protocol::items::TurnItem as CoreTurnItem; use codex_protocol::items::TurnItem as CoreTurnItem;
@@ -355,21 +354,16 @@ pub(crate) async fn apply_bespoke_event_handling(
); );
outgoing.send_server_notification(notification).await; outgoing.send_server_notification(notification).await;
if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress if assessment.status == codex_protocol::protocol::GuardianAssessmentStatus::InProgress
&& match assessment.review_reason { && matches!(
Some(reason) => matches!( assessment.review_reason,
reason, Some(
codex_protocol::approvals::GuardianReviewReason::ElevatedRisk codex_protocol::approvals::GuardianReviewReason::ElevatedRisk
| codex_protocol::approvals::GuardianReviewReason::StaleScore | codex_protocol::approvals::GuardianReviewReason::StaleScore
| codex_protocol::approvals::GuardianReviewReason::IncompatibleCompaction | codex_protocol::approvals::GuardianReviewReason::IncompatibleCompaction
| codex_protocol::approvals::GuardianReviewReason::ScoringFailure | codex_protocol::approvals::GuardianReviewReason::ScoringFailure
| codex_protocol::approvals::GuardianReviewReason::AuthorizationChanged | codex_protocol::approvals::GuardianReviewReason::AuthorizationChanged
), )
// MCP elicitations migrate to per-approval reasons next. )
None => conversation
.thread_extension_data()
.remove::<StrictReviewReason>()
.is_some(),
}
{ {
outgoing outgoing
.send_server_notification(ServerNotification::StrictReviewRequired( .send_server_notification(ServerNotification::StrictReviewRequired(
@@ -0,0 +1,201 @@
//! Verifies independent code-mode and nested-tool policy through the app-server API.
use super::*;
use codex_protocol::openai_models::GuardianModelPolicy;
use codex_protocol::openai_models::GuardianReviewMode;
use pretty_assertions::assert_eq;
use test_case::test_case;
#[test_case(GuardianReviewMode::Adaptive, GuardianReviewMode::Adaptive, 2, 0; "adaptive cells and cua reuse cached scores")]
#[test_case(GuardianReviewMode::Synchronous, GuardianReviewMode::Adaptive, 1, 2; "adaptive cells retain synchronous nested reviews")]
#[test_case(GuardianReviewMode::Adaptive, GuardianReviewMode::Disabled, 1, 1; "shell and cua do not enable code mode")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn model_guardian_policy_scores_code_mode_cells(
computer_use: GuardianReviewMode,
code_mode: GuardianReviewMode,
scores_per_cell: usize,
expected_reviews: usize,
) -> Result<()> {
skip_if_no_network!(Ok(()));
let state = Arc::new(MockResponsesState::default());
let next_cell = Arc::new(Notify::new());
let continue_parent = Arc::clone(&next_cell);
let listener = TcpListener::bind("127.0.0.1:0").await?;
let responses_url = format!("http://{}", listener.local_addr()?);
let router = Router::new()
.route(
"/v1/responses",
get(luna_websocket).post(
move |State(state): State<Arc<MockResponsesState>>, Json(request): Json<Value>| {
let next_cell = Arc::clone(&next_cell);
async move {
if request.pointer("/client_metadata/x-openai-subagent")
== Some(&json!("guardian"))
{
return parent_response(State(state), Json(request))
.await
.into_response();
}
let index = state.parent_requests.fetch_add(/*val*/ 1, Ordering::SeqCst);
if index == 1 {
next_cell.notified().await;
}
let events = if index == 1 {
let cell_id = request["input"]
.as_array()
.expect("parent input")
.iter()
.find(|item| item["type"] == "custom_tool_call_output" && item["call_id"] == "cell-0")
.and_then(|item| item["output"].as_array())
.into_iter()
.flatten()
.filter_map(|item| item["text"].as_str())
.find_map(|text| text.strip_prefix("Script running with cell ID "))
.and_then(|text| text.lines().next())
.expect("first cell yielded");
vec![
responses::ev_response_created("poll"),
responses::ev_function_call(
"poll-cell",
"wait",
&json!({"cell_id": cell_id, "terminate": true}).to_string(),
),
responses::ev_completed("poll"),
]
} else if index < 3 {
let cell = usize::from(index == 2);
let call_id = format!("cell-{cell}");
let pause = if cell == 0 {
"yield_control(); await new Promise(() => {});"
} else {
""
};
vec![
responses::ev_response_created(&call_id),
responses::ev_custom_tool_call(
&call_id,
"exec",
&format!(
"const message = ['browser', '{cell}'].join('-'); text(await tools.mcp__node_repl__js({{message}})); {pause}"
),
),
responses::ev_completed(&call_id),
]
} else {
vec![
responses::ev_assistant_message("done", "done"),
responses::ev_completed("done"),
]
};
(
[(header::CONTENT_TYPE, "text/event-stream")],
responses::sse(events),
)
.into_response()
}
},
),
)
.with_state(Arc::clone(&state));
let responses_server = tokio::spawn(async move {
let _ = axum::serve(listener, router).await;
});
let (mcp_url, mcp_server) =
start_mcp_server_with_tools(&["js"], /*sensitive_action*/ None).await?;
let codex_home = TempDir::new()?;
let analytics_server = responses::start_mock_server().await;
mount_analytics_capture(&analytics_server, codex_home.path()).await?;
MockResponsesConfig::new(&responses_url)
.with_model(MODEL)
.with_provider_config("supports_websockets = false")
.with_approval_policy("on-request")
.with_root_config(&format!(
"approvals_reviewer = \"auto_review\"\nchatgpt_base_url = \"{}\"",
analytics_server.uri(),
))
.with_extra_config(&format!(
"[mcp_servers.node_repl]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[analytics]\nenabled = true"
))
.enable_feature(Feature::GuardianApproval)
.enable_feature(Feature::CodeModeOnly)
.write(codex_home.path())?;
let config = load_default_config_for_test(&codex_home).await;
let mut model = codex_core::test_support::construct_model_info_offline(MODEL, &config);
model.guardian = Some(GuardianModelPolicy {
computer_use: Some(computer_use),
code_mode: Some(code_mode),
shell: Some(GuardianReviewMode::Adaptive),
..Default::default()
});
write_models_cache_with_models(codex_home.path(), vec![model])?;
let mut app_server = TestAppServer::builder()
.with_codex_home(codex_home.path())
.build_initialized_with_timeout(TIMEOUT)
.await?;
let thread = app_server
.start_thread(ThreadStartParams::default())
.await?
.thread;
state.allow_guardian_review.notify_one();
let id = app_server
.send_turn_start_request(TurnStartParams {
thread_id: thread.id,
input: vec![UserInput::Text {
text: USER_CONTEXT.to_owned(),
text_elements: Vec::new(),
}],
..Default::default()
})
.await?;
let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(id)).await??;
// Publish the first cell's scores before polling it and starting the second
// cell. Polling must not classify or invalidate that score. Keep second-cell
// classifiers pending so its approval must use the thread's latest score.
wait_for_luna_request(&state, scores_per_cell - 1).await?;
for completed_scores in 1..=scores_per_cell {
state.allow_luna.notify_one();
timeout(TIMEOUT, async {
loop {
let events = captured_analytics_events(&analytics_server).await;
if events
.iter()
.filter(|event| {
event["event_type"] == "codex_guardian_v2_classification"
&& matches!(
event["event_params"]["outcome"].as_str(),
Some("success" | "superseded")
)
})
.count()
>= completed_scores
{
break;
}
tokio::time::sleep(Duration::from_millis(/*millis*/ 25)).await;
}
})
.await?;
}
continue_parent.notify_one();
let completed: TurnCompletedNotification =
timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??;
assert_eq!(completed.turn.status, TurnStatus::Completed);
wait_for_luna_request(&state, 2 * scores_per_cell - 1).await?;
app_server.shutdown_gracefully().await?;
let requests = state.luna_requests.lock().expect("score requests");
assert_eq!(
(
requests.len(),
state.guardian_reviews.load(Ordering::SeqCst)
),
(2 * scores_per_cell, expected_reviews)
);
assert!(requests.iter().any(|request| {
request
.to_string()
.contains("const message = ['browser', '0'].join('-')")
}));
mcp_server.abort();
responses_server.abort();
Ok(())
}
@@ -0,0 +1,137 @@
//! Exercises model policy, legacy config, and live user modes through the app-server API.
use super::*;
use codex_protocol::openai_models::GuardianModelPolicy;
use codex_protocol::openai_models::GuardianReviewMode;
use pretty_assertions::assert_eq;
use test_case::test_case;
#[derive(Clone, Copy)]
enum ReviewConstraint {
Ordinary,
Sensitive,
}
#[derive(Clone, Copy)]
enum UserMode {
User,
Automatic,
FullAccess,
}
#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::Automatic, false, 1, 0, ReviewConstraint::Ordinary; "model enables async without legacy flag")]
#[test_case(Some(GuardianReviewMode::Synchronous), UserMode::Automatic, true, 0, 1, ReviewConstraint::Ordinary; "model disables async despite legacy flag")]
#[test_case(Some(GuardianReviewMode::Disabled), UserMode::Automatic, true, 0, 0, ReviewConstraint::Ordinary; "model disables ordinary cua review")]
#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::User, true, 0, 0, ReviewConstraint::Ordinary; "user mode never scores")]
#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::FullAccess, true, 0, 0, ReviewConstraint::Ordinary; "full access never scores")]
#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::FullAccess, true, 0, 0, ReviewConstraint::Sensitive; "full access skips sensitive action reviews")]
#[test_case(None, UserMode::Automatic, false, 0, 1, ReviewConstraint::Ordinary; "legacy synchronous config")]
#[test_case(None, UserMode::Automatic, true, 1, 0, ReviewConstraint::Ordinary; "legacy adaptive config")]
#[test_case(Some(GuardianReviewMode::Adaptive), UserMode::Automatic, false, 1, 1, ReviewConstraint::Sensitive; "sensitive elicitation cannot use initial cua allowance")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn model_guardian_policy_controls_cua(
mode: Option<GuardianReviewMode>,
user_mode: UserMode,
legacy_enabled: bool,
expected_scores: usize,
expected_reviews: usize,
constraint: ReviewConstraint,
) -> Result<()> {
skip_if_no_network!(Ok(()));
let state = Arc::new(MockResponsesState {
mcp_server_name: Some("node_repl"),
mcp_tool_sequence: Some(&["js"]),
..Default::default()
});
let listener = TcpListener::bind("127.0.0.1:0").await?;
let responses_url = format!("http://{}", listener.local_addr()?);
let router = Router::new()
.route("/v1/responses", get(luna_websocket).post(parent_response))
.with_state(Arc::clone(&state));
let responses_server = tokio::spawn(async move {
let _ = axum::serve(listener, router).await;
});
let (mcp_url, mcp_server) = start_mcp_server_with_tools(
&["js"],
matches!(constraint, ReviewConstraint::Sensitive).then_some(/*t*/ true),
)
.await?;
let codex_home = TempDir::new()?;
MockResponsesConfig::new(&responses_url)
.with_model(MODEL)
.with_provider_config("supports_websockets = false")
.with_approval_policy("on-request")
.with_root_config("approvals_reviewer = \"auto_review\"")
.with_extra_config(&format!(
"[mcp_servers.node_repl]\nurl = \"{mcp_url}/mcp\"\ndefault_tools_approval_mode = \"auto\"\n\n[features.guardianv2]\nenabled = {legacy_enabled}"
))
.enable_feature(Feature::GuardianApproval)
.write(codex_home.path())?;
let config = load_default_config_for_test(&codex_home).await;
let mut model = codex_core::test_support::construct_model_info_offline(MODEL, &config);
// Exercise both directions of precedence over the legacy CUA bit.
model.node_repl_auto_review_required =
mode.is_none() || mode == Some(GuardianReviewMode::Disabled);
model.guardian = mode.map(|computer_use| GuardianModelPolicy {
computer_use: Some(computer_use),
..Default::default()
});
write_models_cache_with_models(codex_home.path(), vec![model])?;
let mut app_server = TestAppServer::builder()
.with_codex_home(codex_home.path())
.build_initialized_with_timeout(TIMEOUT)
.await?;
let started = app_server
.start_thread(ThreadStartParams {
approvals_reviewer: Some(match user_mode {
UserMode::User => ApprovalsReviewer::User,
UserMode::Automatic | UserMode::FullAccess => ApprovalsReviewer::AutoReview,
}),
approval_policy: matches!(user_mode, UserMode::FullAccess)
.then_some(AskForApproval::Never),
sandbox: matches!(user_mode, UserMode::FullAccess)
.then_some(SandboxMode::DangerFullAccess),
..Default::default()
})
.await?;
let thread = started.thread;
if expected_scores > 0 {
timeout(TIMEOUT, async {
while state.luna_connections.load(Ordering::SeqCst) == 0 {
tokio::task::yield_now().await;
}
})
.await?;
}
state.allow_guardian_review.notify_one();
state.allow_luna.notify_one();
let 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(),
}],
..Default::default()
})
.await?;
let _: TurnStartResponse = timeout(TIMEOUT, app_server.read_response(id)).await??;
let completed: TurnCompletedNotification =
timeout(TIMEOUT, app_server.read_notification("turn/completed")).await??;
assert_eq!(completed.turn.status, TurnStatus::Completed);
if expected_scores > 0 {
wait_for_luna_request(&state, expected_scores - 1).await?;
}
app_server.shutdown_gracefully().await?;
assert_eq!(
(
state.luna_requests.lock().expect("score requests").len(),
state.guardian_reviews.load(Ordering::SeqCst),
state.luna_connections.load(Ordering::SeqCst) > 0,
),
(expected_scores, expected_reviews, expected_scores > 0)
);
mcp_server.abort();
responses_server.abort();
Ok(())
}
@@ -82,6 +82,12 @@ mod history;
#[path = "guardian_v2_model_tests.rs"] #[path = "guardian_v2_model_tests.rs"]
mod model_tests; mod model_tests;
#[path = "guardian_policy_tests.rs"]
mod policy;
#[path = "guardian_code_mode_tests.rs"]
mod code_mode;
const TIMEOUT: Duration = Duration::from_secs(30); const TIMEOUT: Duration = Duration::from_secs(30);
const MODEL: &str = "mock-model"; const MODEL: &str = "mock-model";
const REQUIRED_MODEL: &str = "protected-model"; const REQUIRED_MODEL: &str = "protected-model";
@@ -32,7 +32,7 @@ pub enum NodeReplReviewEvidenceMode {
pub(crate) fn node_repl_review_evidence_mode(turn: &TurnContext) -> NodeReplReviewEvidenceMode { pub(crate) fn node_repl_review_evidence_mode(turn: &TurnContext) -> NodeReplReviewEvidenceMode {
let features = &turn.config.features; let features = &turn.config.features;
if turn.model_info().node_repl_auto_review_required if turn.model_info().computer_use_review_required()
|| features.enabled(Feature::GuardianEnhancedNodeReplTranscripts) || features.enabled(Feature::GuardianEnhancedNodeReplTranscripts)
&& features.enabled(Feature::GuardianNodeReplTranscriptImages) && features.enabled(Feature::GuardianNodeReplTranscriptImages)
{ {
+1
View File
@@ -48,6 +48,7 @@ pub(crate) use review::new_guardian_review_id;
pub(crate) use review::record_guardian_denial_for_test; pub(crate) use review::record_guardian_denial_for_test;
#[cfg(test)] #[cfg(test)]
pub(crate) use review::review_approval_request; pub(crate) use review::review_approval_request;
#[cfg(test)]
pub(crate) use review::review_approval_request_with_cancel; pub(crate) use review::review_approval_request_with_cancel;
pub(crate) use review::routes_approval_policy_to_guardian; pub(crate) use review::routes_approval_policy_to_guardian;
pub(crate) use review::routes_approval_to_guardian; pub(crate) use review::routes_approval_to_guardian;
+7 -1
View File
@@ -5,9 +5,12 @@ use codex_analytics::GuardianReviewFailureReason;
use codex_analytics::GuardianReviewTerminalStatus; use codex_analytics::GuardianReviewTerminalStatus;
use codex_analytics::GuardianReviewTrackContext; use codex_analytics::GuardianReviewTrackContext;
use codex_analytics::GuardianReviewedAction; use codex_analytics::GuardianReviewedAction;
#[cfg(test)]
use codex_analytics::GuardianV2Event; use codex_analytics::GuardianV2Event;
#[cfg(test)]
use codex_analytics::GuardianV2EventKind; use codex_analytics::GuardianV2EventKind;
use codex_core_plugins::PluginCommandAttribution; use codex_core_plugins::PluginCommandAttribution;
#[cfg(test)]
use codex_extension_api::GuardianV2Enabled; use codex_extension_api::GuardianV2Enabled;
use codex_extension_api::ThreadIdleCause; use codex_extension_api::ThreadIdleCause;
use codex_features::Feature; use codex_features::Feature;
@@ -53,6 +56,7 @@ use super::GuardianAssessmentOutcome;
use super::GuardianRejectionCircuitBreakerAction; use super::GuardianRejectionCircuitBreakerAction;
use super::GuardianRejectionCircuitBreakerPolicy; use super::GuardianRejectionCircuitBreakerPolicy;
use super::GuardianReviewContext; use super::GuardianReviewContext;
#[cfg(test)]
use super::approval_request::format_guardian_action_compact; use super::approval_request::format_guardian_action_compact;
use super::approval_request::format_guardian_action_pretty; use super::approval_request::format_guardian_action_pretty;
use super::approval_request::guardian_assessment_action; use super::approval_request::guardian_assessment_action;
@@ -340,6 +344,7 @@ pub(crate) async fn record_guardian_denial_for_test(
/// Runs Guardian unless Full Access or an installed extension resolves the review. /// Runs Guardian unless Full Access or an installed extension resolves the review.
/// Guardian timeouts, review-session failures, and parse failures all block /// Guardian timeouts, review-session failures, and parse failures all block
/// execution, with timeouts surfaced separately from explicit denials. /// execution, with timeouts surfaced separately from explicit denials.
#[cfg(test)]
async fn run_guardian_review( async fn run_guardian_review(
session: Arc<Session>, session: Arc<Session>,
context: GuardianReviewContext, context: GuardianReviewContext,
@@ -927,6 +932,7 @@ pub(crate) async fn review_approval_request(
review.await review.await
} }
#[cfg(test)]
pub(crate) async fn review_approval_request_with_cancel( pub(crate) async fn review_approval_request_with_cancel(
session: &Arc<Session>, session: &Arc<Session>,
context: impl Into<GuardianReviewContext>, context: impl Into<GuardianReviewContext>,
@@ -1038,7 +1044,7 @@ pub(super) async fn guardian_review_session_config(
guardian_reasoning_effort.clone(), guardian_reasoning_effort.clone(),
guardian_model_info.model_messages.as_ref(), guardian_model_info.model_messages.as_ref(),
)?; )?;
if turn.model_info().node_repl_auto_review_required { if turn.model_info().computer_use_review_required() {
spawn_config spawn_config
.features .features
.enable(Feature::RetainClientDeveloperMessages) .enable(Feature::RetainClientDeveloperMessages)
+3 -3
View File
@@ -500,7 +500,7 @@ impl GuardianReviewSessionManager {
parent_context parent_context
.turn() .turn()
.model_info() .model_info()
.node_repl_auto_review_required, .computer_use_review_required(),
) )
.with_node_repl_policy(&session_config.node_repl_policy); .with_node_repl_policy(&session_config.node_repl_policy);
reuse_key.root_authorization_version = root_authorization_version; reuse_key.root_authorization_version = root_authorization_version;
@@ -613,7 +613,7 @@ impl GuardianReviewSessionManager {
.parent_context .parent_context
.turn() .turn()
.model_info() .model_info()
.node_repl_auto_review_required, .computer_use_review_required(),
) )
.with_node_repl_policy(&params.node_repl_policy); .with_node_repl_policy(&params.node_repl_policy);
next_reuse_key.root_authorization_version = root_authorization_version; next_reuse_key.root_authorization_version = root_authorization_version;
@@ -1424,7 +1424,7 @@ async fn ensure_guardian_node_repl_policy(
.parent_context .parent_context
.turn() .turn()
.model_info() .model_info()
.node_repl_auto_review_required .computer_use_review_required()
|| !matches!( || !matches!(
&params.request, &params.request,
GuardianApprovalRequest::McpToolCall { server, tool_name, .. } GuardianApprovalRequest::McpToolCall { server, tool_name, .. }
+3 -3
View File
@@ -17,9 +17,9 @@ use crate::session::session::Session;
/// This lets the extension return AskUser before invoking that renderer. /// This lets the extension return AskUser before invoking that renderer.
#[derive(Clone)] #[derive(Clone)]
pub(crate) struct ReviewAction { pub(crate) struct ReviewAction {
pub(super) action: Result<serde_json::Value, String>, pub(crate) action: Result<serde_json::Value, String>,
pub(super) category: codex_protocol::openai_models::GuardianScope, pub(crate) category: codex_protocol::openai_models::GuardianScope,
pub(super) request: Result<GuardianApprovalRequest, String>, pub(crate) request: Result<GuardianApprovalRequest, String>,
} }
impl From<GuardianApprovalRequest> for ReviewAction { impl From<GuardianApprovalRequest> for ReviewAction {
+95 -101
View File
@@ -807,13 +807,15 @@ async fn review_guardian_mcp_elicitation(
Some(Value::Bool(true)) Some(Value::Bool(true))
); );
if matches!( let mut review_context = crate::guardian::GuardianReviewContext::from(&turn_context);
let strict_auto_review = matches!(
request request
.elicitation .elicitation
.meta() .meta()
.and_then(|meta| meta.get(MCP_ELICITATION_STRICT_AUTO_REVIEW_KEY)), .and_then(|meta| meta.get(MCP_ELICITATION_STRICT_AUTO_REVIEW_KEY)),
Some(Value::Bool(true)) Some(Value::Bool(true))
) { );
let guardian_request = if strict_auto_review {
let connector_id = elicitation_connector_id(&request.elicitation); let connector_id = elicitation_connector_id(&request.elicitation);
let trusted_guardian_request = if request.server_name == CODEX_APPS_MCP_SERVER_NAME { let trusted_guardian_request = if request.server_name == CODEX_APPS_MCP_SERVER_NAME {
let Some(call_id) = request let Some(call_id) = request
@@ -883,109 +885,96 @@ async fn review_guardian_mcp_elicitation(
else { else {
return Ok(None); return Ok(None);
}; };
let guardian_request = trusted_guardian_request.unwrap_or(*guardian_request); trusted_guardian_request.unwrap_or(*guardian_request).into()
let decision = crate::guardian::review_approval_request_with_cancel( } else {
&session, let approval_policy = mcp_config.approval_policy.value();
&turn_context, match approval_policy {
crate::guardian::new_guardian_review_id(), AskForApproval::Never => {
guardian_request, let Some(permission_profile) =
/*retry_reason*/ None, mcp_config.permission_profile_for_server(&request.server_name)
crate::guardian::GuardianReviewOptions { else {
require_guardian: false, return Ok(Some(mcp_elicitation_decline_without_message()));
plugin_attribution_override: None, };
approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, if codex_mcp::mcp_permission_prompt_is_auto_approved(
external_cancel: Some(cancellation_token), approval_policy,
require_synchronous_review, permission_profile,
}, codex_mcp::McpPermissionPromptAutoApproveContext::default(),
) ) && matches!(
.await; &request.elicitation,
Elicitation::Mcp(
return Ok(matches!( rmcp::model::ElicitRequestParams::FormElicitationParams {
decision, requested_schema,
ReviewDecision::Approved ..
| ReviewDecision::Denied { .. } }
| ReviewDecision::TimedOut ) if requested_schema.properties.is_empty()
| ReviewDecision::Abort ) {
) return Ok(Some(ElicitationResponse {
.then(|| { action: ElicitationAction::Accept,
mcp_elicitation_response_from_guardian_decision(decision, turn_context.model_info()) content: Some(serde_json::json!({})),
})); meta: None,
} }));
}
let approval_policy = mcp_config.approval_policy.value();
match approval_policy {
AskForApproval::Never => {
let Some(permission_profile) =
mcp_config.permission_profile_for_server(&request.server_name)
else {
return Ok(Some(mcp_elicitation_decline_without_message())); return Ok(Some(mcp_elicitation_decline_without_message()));
};
if codex_mcp::mcp_permission_prompt_is_auto_approved(
approval_policy,
permission_profile,
codex_mcp::McpPermissionPromptAutoApproveContext::default(),
) && matches!(
&request.elicitation,
Elicitation::Mcp(
rmcp::model::ElicitRequestParams::FormElicitationParams {
requested_schema,
..
}
) if requested_schema.properties.is_empty()
) {
return Ok(Some(ElicitationResponse {
action: ElicitationAction::Accept,
content: Some(serde_json::json!({})),
meta: None,
}));
} }
return Ok(Some(mcp_elicitation_decline_without_message())); AskForApproval::Granular(config) if !config.allows_mcp_elicitations() => {
return Ok(Some(mcp_elicitation_decline_without_message()));
}
AskForApproval::OnRequest
| AskForApproval::UnlessTrusted
| AskForApproval::Granular(_) => {}
} }
AskForApproval::Granular(config) if !config.allows_mcp_elicitations() => {
return Ok(Some(mcp_elicitation_decline_without_message()));
}
AskForApproval::OnRequest | AskForApproval::UnlessTrusted | AskForApproval::Granular(_) => {
}
}
let approvals_reviewer = crate::connectors::mcp_approvals_reviewer_from_layers( let approvals_reviewer = crate::connectors::mcp_approvals_reviewer_from_layers(
&mcp_config.config_layer_stack, &mcp_config.config_layer_stack,
step_settings step_settings
.mcp_approvals_reviewer_override .mcp_approvals_reviewer_override
.unwrap_or(mcp_config.approvals_reviewer), .unwrap_or(mcp_config.approvals_reviewer),
Some(turn_context.model_info().slug.as_str()), Some(turn_context.model_info().slug.as_str()),
request.server_name.as_str(), request.server_name.as_str(),
elicitation_connector_id(&request.elicitation), elicitation_connector_id(&request.elicitation),
/*link_id*/ None, /*link_id*/ None,
); );
if !crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) { review_context.approval_policy = approval_policy;
return Ok(None); review_context.approvals_reviewer = approvals_reviewer;
} match guardian_elicitation_review_request(&request, originating_call_id) {
GuardianElicitationReview::NotRequested => return Ok(None),
let guardian_request = match guardian_elicitation_review_request(&request, originating_call_id) GuardianElicitationReview::Decline(reason) => {
{ let Elicitation::Mcp(elicitation) = &request.elicitation else {
GuardianElicitationReview::NotRequested => return Ok(None), return Ok(None);
GuardianElicitationReview::Decline(reason) => { };
warn!( crate::guardian::ReviewAction {
server_name = %request.server_name, action: serde_json::to_value(elicitation)
request_id = %mcp_elicitation_request_id(&request.request_id), .map(|elicitation| {
reason, serde_json::json!({
"declining Guardian MCP elicitation before review" "tool": "mcp_elicitation",
); "server": request.server_name,
return Ok(Some(mcp_elicitation_decline_without_message())); "request": elicitation,
})
})
.map_err(|error| error.to_string()),
category: codex_protocol::openai_models::GuardianScope::for_mcp_server(
&request.server_name,
),
request: Err(reason.to_owned()),
}
}
GuardianElicitationReview::ApprovalRequest(guardian_request) => {
(*guardian_request).into()
}
} }
GuardianElicitationReview::ApprovalRequest(guardian_request) => *guardian_request,
}; };
let declined_reason = guardian_request.request.as_ref().err().cloned();
let review_id = crate::guardian::new_guardian_review_id(); let decision = crate::guardian::decide_approval(
let decision = crate::guardian::review_approval_request_with_cancel( session,
&session, review_context,
&turn_context, crate::guardian::new_guardian_review_id(),
review_id,
guardian_request, guardian_request,
/*retry_reason*/ None, crate::tools::sandboxing::ApprovalRequestReasons {
approval: None,
retry: None,
},
crate::guardian::GuardianReviewOptions { crate::guardian::GuardianReviewOptions {
require_guardian: false, require_guardian: strict_auto_review,
plugin_attribution_override: None, plugin_attribution_override: None,
approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn,
external_cancel: Some(cancellation_token), external_cancel: Some(cancellation_token),
@@ -993,10 +982,15 @@ async fn review_guardian_mcp_elicitation(
}, },
) )
.await; .await;
Ok(Some(mcp_elicitation_response_from_guardian_decision( Ok(decision.map(|decision| {
decision, if let Some(reason) = declined_reason {
turn_context.model_info(), warn!(server_name = %request.server_name,
))) request_id = %mcp_elicitation_request_id(&request.request_id), reason,
"declining Guardian MCP elicitation before review");
return mcp_elicitation_decline_without_message();
}
mcp_elicitation_response_from_guardian_decision(decision, turn_context.model_info())
}))
} }
fn guardian_elicitation_review_request( fn guardian_elicitation_review_request(
+1 -1
View File
@@ -218,7 +218,7 @@ impl TurnMetadataState {
turn_id, turn_id,
sandbox_tags, sandbox_tags,
auto_review_enabled, auto_review_enabled,
node_repl_auto_review_required: model_info.node_repl_auto_review_required, node_repl_auto_review_required: model_info.computer_use_review_required(),
node_repl_disabled: model_info.node_repl_disabled, node_repl_disabled: model_info.node_repl_disabled,
enriched_workspaces: RwLock::new(None), enriched_workspaces: RwLock::new(None),
tool_namespaces_info: RwLock::new(None), tool_namespaces_info: RwLock::new(None),
@@ -11,7 +11,6 @@ use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::GuardianAssessmentAction; use codex_protocol::protocol::GuardianAssessmentAction;
use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::GuardianAssessmentStatus;
use codex_protocol::protocol::Op; use codex_protocol::protocol::Op;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::ThreadSettingsOverrides; use codex_protocol::protocol::ThreadSettingsOverrides;
use codex_protocol::user_input::UserInput; use codex_protocol::user_input::UserInput;
use core_test_support::responses; use core_test_support::responses;
@@ -19,6 +18,7 @@ use core_test_support::skip_if_no_network;
use core_test_support::skip_if_wine_exec; use core_test_support::skip_if_wine_exec;
use core_test_support::test_codex::local_selections; use core_test_support::test_codex::local_selections;
use core_test_support::test_codex::test_codex; use core_test_support::test_codex::test_codex;
use core_test_support::wait_for_event;
use core_test_support::wait_for_mcp_server; use core_test_support::wait_for_mcp_server;
use pretty_assertions::assert_eq; use pretty_assertions::assert_eq;
use serde_json::Value; use serde_json::Value;
@@ -72,6 +72,12 @@ for line in sys.stdin:
"_meta": {"codex_request_type": "approval_request", "_meta": {"codex_request_type": "approval_request",
"codex_approval_kind": "mcp_tool_call", "tool_name": "write_record", "codex_approval_kind": "mcp_tool_call", "tool_name": "write_record",
**meta, **invocation_meta}}}) **meta, **invocation_meta}}})
if len(sys.argv) > 3:
for elicitation in pending_elicitations:
params = elicitation["params"]
params.update(json.loads(sys.argv[3]))
if params.get("mode") == "url":
params.pop("requestedSchema")
send(pending_elicitations.pop(0)) send(pending_elicitations.pop(0))
continue continue
elif method is None and str(request.get("id", "")).startswith("server-approval"): elif method is None and str(request.get("id", "")).startswith("server-approval"):
@@ -224,14 +230,12 @@ async fn server_initiated_mcp_elicitation_can_require_synchronous_auto_review(
struct AutoApprovingReviewContributor; struct AutoApprovingReviewContributor;
impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor { impl codex_extension_api::ApprovalReviewContributor for AutoApprovingReviewContributor {
fn fast_decision<'a>( fn decide<'a>(
&'a self, &'a self,
_session_store: &'a codex_extension_api::ExtensionData, _input: &'a codex_extension_api::ApprovalDecisionInput<'_>,
_thread_store: &'a codex_extension_api::ExtensionData, ) -> codex_extension_api::ExtensionFuture<'a, Option<codex_extension_api::ApprovalDecision>>
_prompt: &'a str, {
_extension_metrics: Option<Arc<dyn codex_extension_api::ExtensionMetrics>>, Box::pin(async { Some(codex_extension_api::ApprovalDecision::Allow) })
) -> codex_extension_api::ExtensionFuture<'a, Option<ReviewDecision>> {
Box::pin(async { Some(ReviewDecision::Approved) })
} }
} }
@@ -653,3 +657,116 @@ async fn node_elicitations_attribute_independent_reviews_without_changing_action
test.codex.shutdown_and_wait().await?; test.codex.shutdown_and_wait().await?;
Ok(()) Ok(())
} }
#[test_case("form"; "nonempty_form")]
#[test_case("url"; "url")]
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn user_review_preserves_unsupported_guardian_elicitations(mode: &str) -> Result<()> {
skip_if_no_network!(Ok(()));
skip_if_wine_exec!(Ok(()), "the MCP fixture requires a host Python interpreter");
let server = responses::start_mock_server().await;
let form_schema = json!({
"type": "object", "properties": {"name": {"type": "string"}}, "required": ["name"]
});
let params = if mode == "url" {
json!({"mode": "url", "url": "https://example.com/approve", "elicitationId": "approval"})
} else {
json!({"requestedSchema": form_schema})
};
let mcp_servers = serde_json::from_value(json!({
"elicitation": {
"command": if cfg!(windows) { "python" } else { "python3" },
"args": ["-u", "-c", ELICITATION_SERVER, "{}", "", params.to_string()],
"default_tools_approval_mode": "approve",
}
}))?;
let test = test_codex()
.with_config(move |config| {
config.approvals_reviewer = ApprovalsReviewer::User;
config.permissions.approval_policy = Constrained::allow_any(AskForApproval::Granular(
codex_protocol::protocol::GranularApprovalConfig {
sandbox_approval: false,
rules: false,
skill_approval: false,
request_permissions: false,
mcp_elicitations: true,
},
));
config
.mcp_servers
.set(mcp_servers)
.expect("set MCP fixture");
})
.build_with_auto_env(&server)
.await?;
wait_for_mcp_server(&test.codex, "elicitation").await?;
responses::mount_sse_once(
&server,
responses::sse(vec![
responses::ev_function_call_with_namespace(
"eliciting-tool",
"mcp__elicitation",
"request_approval",
"{}",
),
responses::ev_completed("parent-tool"),
]),
)
.await;
let parent = responses::mount_sse_once(
&server,
responses::sse(vec![responses::ev_completed("parent-complete")]),
)
.await;
test.codex
.start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text {
text: "Run the tool and ask me for its approval.".into(),
text_elements: Vec::new(),
}]))
.await?;
let event = wait_for_event(&test.codex, |event| {
matches!(
event,
EventMsg::ElicitationRequest(_)
| EventMsg::GuardianAssessment(_)
| EventMsg::TurnComplete(_)
)
})
.await;
let EventMsg::ElicitationRequest(request) = event else {
panic!("expected a user elicitation, got {event:?}");
};
match &request.request {
codex_protocol::approvals::ElicitationRequest::Form {
requested_schema, ..
} => {
assert_eq!(requested_schema, &form_schema);
}
codex_protocol::approvals::ElicitationRequest::Url { url, .. } => {
assert_eq!(url, "https://example.com/approve");
}
other => panic!("unexpected elicitation: {other:?}"),
}
test.codex
.submit(Op::ResolveElicitation {
server_name: request.server_name,
request_id: request.id,
decision: codex_protocol::approvals::ElicitationAction::Decline,
content: None,
meta: None,
})
.await?;
wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::TurnComplete(_))
})
.await;
assert!(
parent
.single_request()
.function_call_output("eliciting-tool")
.to_string()
.contains("decline")
);
test.codex.shutdown_and_wait().await?;
Ok(())
}