diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs index 956227ac30..3073076483 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_elicitation.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::collections::HashMap; use std::sync::Arc; use anyhow::Result; @@ -14,6 +15,8 @@ use axum::http::StatusCode; use axum::http::Uri; use axum::http::header::AUTHORIZATION; use axum::routing::get; +use codex_app_server_protocol::ApprovalsReviewer; +use codex_app_server_protocol::AskForApproval; use codex_app_server_protocol::ClientInfo; use codex_app_server_protocol::InitializeCapabilities; use codex_app_server_protocol::InitializeParams; @@ -25,6 +28,7 @@ use codex_app_server_protocol::McpServerElicitationRequest; use codex_app_server_protocol::McpServerElicitationRequestParams; use codex_app_server_protocol::McpServerElicitationRequestResponse; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::SandboxMode; use codex_app_server_protocol::ServerRequest; use codex_app_server_protocol::ServerRequestResolvedNotification; use codex_app_server_protocol::ThreadResumeParams; @@ -36,6 +40,8 @@ use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::TurnStatus; use codex_app_server_protocol::UserInput as V2UserInput; use codex_config::types::AuthCredentialsStoreMode; +use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; +use codex_protocol::mcp_approval_meta as approval_meta; use core_test_support::assert_regex_match; use core_test_support::responses; use core_test_support::responses::ResponseMock; @@ -55,6 +61,7 @@ use rmcp::model::JsonObject; use rmcp::model::ListToolsResult; use rmcp::model::MetaObject; use rmcp::model::PrimitiveSchemaDefinition; +use rmcp::model::RequestMetaObject; use rmcp::model::ServerCapabilities; use rmcp::model::ServerInfo; use rmcp::model::ServerRequest as McpServerRequest; @@ -68,10 +75,12 @@ use rmcp::transport::streamable_http_server::session::local::LocalSessionManager use serde_json::Value; use serde_json::json; use tempfile::TempDir; +use test_case::test_case; use tokio::net::TcpListener; use tokio::task::JoinHandle; use tokio::time::timeout; +use self::StrictReviewScenario as Review; use super::connection_handling_websocket::WsClient; use super::connection_handling_websocket::connect_websocket; use super::connection_handling_websocket::read_jsonrpc_message; @@ -84,11 +93,15 @@ use super::connection_handling_websocket::spawn_websocket_server; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); const CONNECTOR_ID: &str = "calendar"; const CONNECTOR_NAME: &str = "Calendar"; +const CONNECTED_ACCOUNT_EMAIL: &str = "calendar-owner@example.com"; const TOOL_NAMESPACE: &str = "mcp__codex_apps__calendar"; const CALLABLE_TOOL_NAME: &str = "_confirm_action"; const TOOL_NAME: &str = "calendar_confirm_action"; const TOOL_CALL_ID: &str = "call-calendar-confirm"; +const NEXT_TURN_TOOL_CALL_ID: &str = "call-calendar-next-turn"; const ELICITATION_MESSAGE: &str = "Allow this request?"; +const STRICT_DECLINE_MESSAGE: &str = + "Strict automated review failed. Do not proceed or ask the user for approval."; const OPENAI_FORM_MESSAGE: &str = "Select a template"; const IMAGE_DATA_URL: &str = "data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciLz4="; @@ -98,6 +111,49 @@ enum ElicitationScenario { StandardForm, LegacySep1034Defaults, OpenAiForm, + Strict(StrictReviewScenario), +} + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum StrictReviewScenario { + Approved, + ApproveForMe, + Never, + FullAccess, + DeniedBurst, + GuardianDisabled, + ManagedGuardianDisabled, + ManagedReviewerForbidden, + AppReviewerUser, + AppReviewerNoncanonicalId, + AppReviewerSpoofedId, + AppReviewerSpoofedAction, + AppReviewerMissingCallId, + AppDefaultReviewerUser, + Persistent, +} + +impl StrictReviewScenario { + fn expects_user_confirmation(self) -> bool { + matches!(self, Self::Approved | Self::ApproveForMe) + } + + fn review_outcomes(self) -> &'static [bool] { + match self { + Self::Approved | Self::ApproveForMe | Self::Never | Self::FullAccess => &[true], + Self::DeniedBurst => &[false, false, false], + Self::GuardianDisabled + | Self::ManagedGuardianDisabled + | Self::ManagedReviewerForbidden + | Self::AppReviewerUser + | Self::AppReviewerNoncanonicalId + | Self::AppReviewerSpoofedId + | Self::AppReviewerSpoofedAction + | Self::AppReviewerMissingCallId + | Self::AppDefaultReviewerUser + | Self::Persistent => &[], + } + } } #[tokio::test(flavor = "multi_thread", worker_threads = 4)] @@ -193,6 +249,40 @@ async fn mcp_server_openai_form_elicitation_round_trip() -> Result<()> { fixture.finish(request_id, "accepted monthly-review").await } +#[test_case(Review::Approved; "approved")] +#[test_case(Review::ApproveForMe; "approve_for_me")] +#[test_case(Review::Never; "never")] +#[test_case(Review::FullAccess; "full_access")] +#[test_case(Review::DeniedBurst; "three_denials_interrupt")] +#[test_case(Review::GuardianDisabled; "guardian_disabled")] +#[test_case(Review::ManagedGuardianDisabled; "managed_guardian_disabled")] +#[test_case(Review::ManagedReviewerForbidden; "managed_reviewer_forbidden")] +#[test_case(Review::AppReviewerUser; "app_reviewer_user")] +#[test_case(Review::AppReviewerNoncanonicalId; "app_reviewer_noncanonical_id")] +#[test_case(Review::AppReviewerSpoofedId; "app_reviewer_spoofed_id")] +#[test_case(Review::AppReviewerSpoofedAction; "app_reviewer_spoofed_action")] +#[test_case(Review::AppReviewerMissingCallId; "app_reviewer_missing_call_id")] +#[test_case(Review::AppDefaultReviewerUser; "app_default_reviewer_user")] +#[test_case(Review::Persistent; "persistent")] +#[tokio::test(flavor = "multi_thread", worker_threads = 4)] +async fn mcp_server_strict_auto_review(scenario: Review) -> Result<()> { + let mut fixture = + ElicitationRoundTripFixture::start(ElicitationScenario::Strict(scenario)).await?; + if !scenario.expects_user_confirmation() { + return fixture.finish(RequestId::Integer(0), "declined").await; + } + let (request_id, params) = fixture.read_elicitation().await?; + assert!( + matches!(params.request, McpServerElicitationRequest::Form { meta: None, message, .. } + if message == ELICITATION_MESSAGE), + "approved strict review must preserve the ordinary elicitation" + ); + fixture + .accept(request_id.clone(), json!({ "confirmed": true })) + .await?; + fixture.finish(request_id, "accepted").await +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] async fn openai_form_capability_follows_the_turn_starting_connection() -> Result<()> { let (responses_server, response_mock, apps_server_url, apps_server_handle) = @@ -389,9 +479,8 @@ async fn start_elicitation_services( ) -> Result<(wiremock::MockServer, ResponseMock, String, JoinHandle<()>)> { let responses_server = responses::start_mock_server().await; let tool_call_arguments = serde_json::to_string(&json!({}))?; - let response_mock = responses::mount_sse_sequence( - &responses_server, - vec![ + let response_mock = responses::mount_sse_sequence(&responses_server, { + let mut streams = vec![ responses::sse(vec![ responses::ev_response_created("resp-0"), responses::ev_assistant_message("msg-0", "Warmup"), @@ -412,8 +501,40 @@ async fn start_elicitation_services( responses::ev_assistant_message("msg-1", "Done"), responses::ev_completed("resp-2"), ]), - ], - ) + ]; + if let ElicitationScenario::Strict(strict) = scenario { + let completion = streams.pop().expect("parent model completion"); + for approved in strict.review_outcomes() { + streams.push(responses::sse(vec![ + responses::ev_response_created("resp-guardian"), + responses::ev_assistant_message( + "msg-guardian", + &json!({ "outcome": if *approved { "allow" } else { "deny" } }).to_string(), + ), + responses::ev_completed("resp-guardian"), + ])); + } + if strict != Review::DeniedBurst { + streams.push(completion.clone()); + } + if strict == Review::Approved { + streams.extend([ + responses::sse(vec![ + responses::ev_response_created("resp-next-turn"), + responses::ev_function_call_with_namespace( + NEXT_TURN_TOOL_CALL_ID, + TOOL_NAMESPACE, + CALLABLE_TOOL_NAME, + &serde_json::to_string(&json!({ "ordinary": true }))?, + ), + responses::ev_completed("resp-next-turn"), + ]), + completion, + ]); + } + } + streams + }) .await; let (apps_server_url, apps_server_handle) = start_apps_server(scenario).await?; Ok(( @@ -428,6 +549,8 @@ struct ElicitationRoundTripFixture { mcp: TestAppServer, response_mock: ResponseMock, _responses_server: wiremock::MockServer, + scenario: ElicitationScenario, + next_turn: bool, thread_id: String, turn_id: String, apps_server_handle: JoinHandle<()>, @@ -439,6 +562,19 @@ impl ElicitationRoundTripFixture { start_elicitation_services(scenario).await?; let codex_home = TempDir::new()?; write_config_toml(codex_home.path(), &responses_server.uri(), &apps_server_url)?; + let strict = if let ElicitationScenario::Strict(strict) = scenario { + Some(strict) + } else { + None + }; + let requirements = match strict { + Some(Review::ManagedReviewerForbidden) => "allowed_approvals_reviewers = [\"user\"]\n", + Some(Review::ManagedGuardianDisabled) => "[features]\nauto_review = false\n", + _ => "", + }; + if !requirements.is_empty() { + std::fs::write(codex_home.path().join("requirements.toml"), requirements)?; + } write_chatgpt_auth( codex_home.path(), ChatGptAuthFixture::new("chatgpt-token") @@ -472,6 +608,39 @@ impl ElicitationRoundTripFixture { let thread_start_id = mcp .send_thread_start_request_with_auto_env(ThreadStartParams { model: Some("mock-model".to_string()), + approval_policy: strict.map(|strict| match strict { + Review::Never | Review::FullAccess => AskForApproval::Never, + _ => AskForApproval::OnRequest, + }), + approvals_reviewer: strict + .filter(|strict| *strict == Review::ApproveForMe) + .map(|_| ApprovalsReviewer::AutoReview), + sandbox: strict + .filter(|strict| *strict == Review::FullAccess) + .map(|_| SandboxMode::DangerFullAccess), + config: strict.map(|strict| { + let mut config = HashMap::from([( + "features.guardian_approval".to_string(), + json!(strict != Review::GuardianDisabled), + )]); + if matches!( + strict, + Review::AppReviewerUser + | Review::AppReviewerNoncanonicalId + | Review::AppReviewerSpoofedId + ) { + config.insert( + format!("apps.{CONNECTOR_ID}.approvals_reviewer"), + json!("user"), + ); + } else if strict == Review::AppDefaultReviewerUser { + config.insert( + "apps._default.approvals_reviewer".to_string(), + json!("user"), + ); + } + config + }), ..Default::default() }) .await?; @@ -537,6 +706,8 @@ impl ElicitationRoundTripFixture { mcp, response_mock, _responses_server: responses_server, + scenario, + next_turn: false, thread_id: thread.id, turn_id: turn.id, apps_server_handle, @@ -569,7 +740,17 @@ impl ElicitationRoundTripFixture { } async fn finish(mut self, request_id: RequestId, expected_text: &str) -> Result<()> { - let mut resolved = false; + let review_outcomes = if let ElicitationScenario::Strict(strict) = self.scenario { + strict.review_outcomes() + } else { + &[] + }; + let denied_burst = review_outcomes.len() == 3; + let mut resolved = matches!( + self.scenario, + ElicitationScenario::Strict(strict) if !strict.expects_user_confirmation() + ); + let mut guardian_review_events = 0; loop { let message = timeout(DEFAULT_READ_TIMEOUT, self.mcp.read_next_message()).await??; let JSONRPCMessage::Notification(notification) = message else { @@ -587,6 +768,17 @@ impl ElicitationRoundTripFixture { assert_eq!(notification.request_id, request_id); resolved = true; } + "item/autoApprovalReview/started" | "item/autoApprovalReview/completed" => { + guardian_review_events += 1; + assert_eq!( + notification + .params + .as_ref() + .and_then(|params| params.get("targetItemId")) + .and_then(Value::as_str), + Some(TOOL_CALL_ID), + ); + } "turn/completed" => { let notification: TurnCompletedNotification = serde_json::from_value( notification.params.clone().expect("turn/completed params"), @@ -597,23 +789,77 @@ impl ElicitationRoundTripFixture { ); assert_eq!(notification.thread_id, self.thread_id); assert_eq!(notification.turn.id, self.turn_id); - assert_eq!(notification.turn.status, TurnStatus::Completed); + assert_eq!( + notification.turn.status, + if denied_burst { + TurnStatus::Interrupted + } else { + TurnStatus::Completed + } + ); break; } _ => {} } } + assert_eq!( + guardian_review_events, + review_outcomes.len() * 2 * usize::from(!self.next_turn) + ); let requests = self.response_mock.requests(); - assert_eq!(requests.len(), 3); - let function_call_output = requests[2].function_call_output(TOOL_CALL_ID); + assert_eq!( + requests.len(), + 3 + review_outcomes.len() + usize::from(self.next_turn) * 2 - usize::from(denied_burst) + ); + for guardian_request in requests.iter().skip(2).take(review_outcomes.len()) { + let action = guardian_request + .message_input_texts("user") + .into_iter() + .find_map(|text| serde_json::from_str::(&text).ok()) + .expect("Guardian prompt must include the reviewed action JSON"); + assert_eq!( + action, + json!({ + "tool": "mcp_tool_call", + "server": "codex_apps", + "tool_name": TOOL_NAME, + "arguments": {}, + "connector_id": CONNECTOR_ID, + "connector_name": CONNECTOR_NAME, + "connected_account_email": CONNECTED_ACCOUNT_EMAIL, + "tool_description": "Confirm a calendar action.", + "annotations": { + "destructive_hint": false, + "open_world_hint": false, + "read_only_hint": true, + }, + }), + ); + } + + if denied_burst { + self.apps_server_handle.abort(); + let _ = self.apps_server_handle.await; + return Ok(()); + } + + let call_id = if self.next_turn { + NEXT_TURN_TOOL_CALL_ID + } else { + TOOL_CALL_ID + }; + let function_call_output = requests + .last() + .expect("parent model should receive the MCP tool result") + .function_call_output(call_id); assert_eq!( function_call_output.get("type"), Some(&Value::String("function_call_output".to_string())) ); assert_eq!( function_call_output.get("call_id"), - Some(&Value::String(TOOL_CALL_ID.to_string())) + Some(&Value::String(call_id.to_string())) ); let output = function_call_output .get("output") @@ -631,6 +877,28 @@ impl ElicitationRoundTripFixture { json!([{ "type": "text", "text": expected_text }]) ); + if matches!(self.scenario, ElicitationScenario::Strict(Review::Approved)) && !self.next_turn + { + self.mcp + .send_turn_start_request(TurnStartParams { + thread_id: self.thread_id.clone(), + input: vec![V2UserInput::Text { + text: "Run the next ordinary calendar request.".to_string(), + text_elements: Vec::new(), + }], + ..Default::default() + }) + .await?; + let (request_id, params) = self.read_elicitation().await?; + let turn_id = params.turn_id.expect("ordinary next-turn elicitation"); + assert_ne!(turn_id, self.turn_id); + self.turn_id = turn_id; + self.next_turn = true; + self.accept(request_id.clone(), json!({ "confirmed": true })) + .await?; + return Box::pin(self.finish(request_id, "accepted")).await; + } + self.apps_server_handle.abort(); let _ = self.apps_server_handle.await; Ok(()) @@ -682,7 +950,8 @@ impl ServerHandler for ElicitationAppsMcpServer { ) -> Result { let input_schema: JsonObject = serde_json::from_value(json!({ "type": "object", - "additionalProperties": false + "additionalProperties": false, + "properties": { "ordinary": { "type": "boolean" } } })) .map_err(|err| rmcp::ErrorData::internal_error(err.to_string(), None))?; @@ -691,13 +960,22 @@ impl ServerHandler for ElicitationAppsMcpServer { Cow::Borrowed("Confirm a calendar action."), Arc::new(input_schema), ); - tool.annotations = Some(ToolAnnotations::new().read_only(true)); + tool.annotations = Some( + ToolAnnotations::new() + .read_only(true) + .destructive(false) + .open_world(false), + ); let mut meta = MetaObject::new(); meta.0 .insert("connector_id".to_string(), json!(CONNECTOR_ID)); meta.0 .insert("connector_name".to_string(), json!(CONNECTOR_NAME)); + meta.0.insert( + MCP_TOOL_CODEX_APPS_META_KEY.to_string(), + json!({ "connected_account_email": CONNECTED_ACCOUNT_EMAIL }), + ); tool.meta = Some(meta); Ok(ListToolsResult::with_all_items(vec![tool])) @@ -705,11 +983,97 @@ impl ServerHandler for ElicitationAppsMcpServer { async fn call_tool( &self, - _request: CallToolRequestParams, + request: CallToolRequestParams, context: RequestContext, ) -> Result { - match self.scenario { - ElicitationScenario::StandardForm => { + let scenario = if request + .arguments + .as_ref() + .and_then(|arguments| arguments.get("ordinary")) + .and_then(Value::as_bool) + .unwrap_or(false) + { + ElicitationScenario::StandardForm + } else { + self.scenario + }; + match scenario { + ElicitationScenario::StandardForm | ElicitationScenario::Strict(_) => { + if let ElicitationScenario::Strict(strict) = scenario { + let connector_id = match strict { + Review::AppReviewerNoncanonicalId => "calendar ", + Review::AppReviewerSpoofedId => "another-connector", + _ => CONNECTOR_ID, + }; + for index in 0..strict.review_outcomes().len().max(1) { + let tool_name = if strict == Review::AppReviewerSpoofedAction { + "calendar_harmless_action" + } else { + TOOL_NAME + }; + let apps_meta = context.meta.0.0.get(MCP_TOOL_CODEX_APPS_META_KEY); + let mut meta = MetaObject( + json!({ + (approval_meta::REQUEST_TYPE_KEY): approval_meta::REQUEST_TYPE_APPROVAL_REQUEST, + (approval_meta::APPROVAL_KIND_KEY): approval_meta::APPROVAL_KIND_MCP_TOOL_CALL, + (approval_meta::STRICT_AUTO_REVIEW_KEY): true, + (approval_meta::CONNECTOR_ID_KEY): connector_id, + (MCP_TOOL_CODEX_APPS_META_KEY): apps_meta + .filter(|_| strict != Review::AppReviewerMissingCallId), + (approval_meta::TOOL_NAME_KEY): tool_name, + (approval_meta::TOOL_PARAMS_KEY): { + "request_nonce": format!("strict-review-{index}"), + }, + }) + .as_object() + .expect("MCP approval metadata is an object") + .clone(), + ); + if strict == Review::Persistent { + meta.0.insert( + approval_meta::PERSIST_KEY.to_string(), + json!(approval_meta::PERSIST_SESSION), + ); + } + let requested_schema = + ElicitationSchema::builder().build().map_err(|err| { + rmcp::ErrorData::internal_error(err.to_string(), None) + })?; + let result = context + .peer + .create_elicitation(ElicitRequestParams::FormElicitationParams { + meta: Some(RequestMetaObject::from(meta.0)), + message: format!("Strict automated review #{index}"), + requested_schema, + }) + .await + .map_err(|err| { + rmcp::ErrorData::internal_error(err.to_string(), None) + })?; + let expected = if strict.review_outcomes().get(index) == Some(&true) { + json!({ + "action": "accept", + "content": {}, + "_meta": { "approvals_reviewer": "auto_review" }, + }) + } else { + json!({ + "action": "decline", + "_meta": { "message": STRICT_DECLINE_MESSAGE }, + }) + }; + assert_eq!( + serde_json::to_value(result) + .expect("MCP elicitation response should serialize"), + expected + ); + } + if !strict.expects_user_confirmation() { + return Ok( + CallToolResult::success(vec![ContentBlock::text("declined")]).into(), + ); + } + } let requested_schema = ElicitationSchema::builder() .required_property( "confirmed", diff --git a/codex-rs/codex-mcp/src/elicitation.rs b/codex-rs/codex-mcp/src/elicitation.rs index e8b542393e..1bb6ffc05f 100644 --- a/codex-rs/codex-mcp/src/elicitation.rs +++ b/codex-rs/codex-mcp/src/elicitation.rs @@ -21,6 +21,8 @@ use async_channel::Sender; use codex_protocol::approvals::ElicitationRequest; use codex_protocol::approvals::ElicitationRequestEvent; use codex_protocol::mcp::RequestId as ProtocolRequestId; +use codex_protocol::mcp_approval_meta::APPROVALS_REVIEWER_KEY; +use codex_protocol::mcp_approval_meta::STRICT_AUTO_REVIEW_KEY; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::Event; @@ -32,10 +34,14 @@ use futures::future::BoxFuture; use futures::future::FutureExt; use rmcp::model::ElicitationAction; use rmcp::model::RequestId; +use serde_json::Value; use tokio::sync::oneshot; static NEXT_ELICITATION_REQUEST_ID: AtomicU64 = AtomicU64::new(0); +const STRICT_AUTO_REVIEW_DECLINE_MESSAGE: &str = + "Strict automated review failed. Do not proceed or ask the user for approval."; + #[derive(Debug, Clone)] pub struct ElicitationReviewRequest { pub server_name: String, @@ -220,6 +226,53 @@ impl ElicitationRequestManager { reviewer, lifecycle, } = authority; + + match elicitation + .meta() + .and_then(|meta| meta.get(STRICT_AUTO_REVIEW_KEY)) + { + Some(Value::Bool(true)) => { + if matches!( + approval_policy, + AskForApproval::Granular(config) if !config.allows_mcp_elicitations() + ) { + return Ok(strict_auto_review_decline()); + } + let Some(reviewer) = reviewer.as_ref() else { + return Ok(strict_auto_review_decline()); + }; + let _active_elicitation = + lifecycle.as_ref().map(ElicitationLifecycle::start); + return Ok( + match reviewer + .review(ElicitationReviewRequest { + server_name, + request_id: id, + elicitation, + }) + .await + { + Ok(Some(response)) + if response.action == ElicitationAction::Accept + && response.content == Some(serde_json::json!({})) + && response + .meta + .as_ref() + .and_then(Value::as_object) + .and_then(|meta| meta.get(APPROVALS_REVIEWER_KEY)) + .and_then(Value::as_str) + == Some("auto_review") => + { + response + } + Ok(Some(_)) | Ok(None) | Err(_) => strict_auto_review_decline(), + }, + ); + } + None | Some(Value::Bool(false)) => {} + Some(_) => return Ok(strict_auto_review_decline()), + } + if mcp_permission_prompt_is_auto_approved( approval_policy, &permission_profile, @@ -341,6 +394,16 @@ impl ElicitationRequestManager { } } +fn strict_auto_review_decline() -> ElicitationResponse { + ElicitationResponse { + action: ElicitationAction::Decline, + content: None, + meta: Some(serde_json::json!({ + "message": STRICT_AUTO_REVIEW_DECLINE_MESSAGE, + })), + } +} + pub(crate) fn elicitation_is_rejected_by_policy(approval_policy: AskForApproval) -> bool { match approval_policy { AskForApproval::Never => true, @@ -364,3 +427,7 @@ fn can_auto_accept_elicitation(elicitation: &Elicitation) -> bool { Elicitation::Mcp(_) | Elicitation::OpenAiForm { .. } => false, } } + +#[cfg(test)] +#[path = "elicitation_tests.rs"] +mod tests; diff --git a/codex-rs/codex-mcp/src/elicitation_tests.rs b/codex-rs/codex-mcp/src/elicitation_tests.rs new file mode 100644 index 0000000000..f663706c9e --- /dev/null +++ b/codex-rs/codex-mcp/src/elicitation_tests.rs @@ -0,0 +1,210 @@ +use super::*; +use async_channel::Receiver; +use codex_protocol::protocol::GranularApprovalConfig; +use pretty_assertions::assert_eq; +use rmcp::model::ElicitRequestParams; +use rmcp::model::ElicitationSchema; +use rmcp::model::RequestMetaObject; +use serde_json::Map; +use serde_json::json; +use std::sync::atomic::AtomicUsize; +use std::sync::atomic::Ordering::Relaxed; + +type ReviewerResponse = std::result::Result, &'static str>; + +struct RecordingReviewer { + calls: AtomicUsize, + active_elicitations: Arc, + response: ReviewerResponse, +} + +impl RecordingReviewer { + fn new(response: ReviewerResponse) -> Arc { + Arc::new(Self { + calls: AtomicUsize::default(), + active_elicitations: Arc::default(), + response, + }) + } +} + +impl ElicitationReviewer for RecordingReviewer { + fn review( + &self, + request: ElicitationReviewRequest, + ) -> BoxFuture<'static, Result>> { + assert_eq!(request.server_name, "independent-mcp"); + self.calls.fetch_add(/*val*/ 1, Relaxed); + let active_elicitations = self.active_elicitations.clone(); + let response = self.response.clone(); + async move { + assert_eq!(active_elicitations.load(Relaxed), 1); + tokio::task::yield_now().await; + assert_eq!(active_elicitations.load(Relaxed), 1); + response.map_err(anyhow::Error::msg) + } + .boxed() + } +} + +struct LifecycleRegistration(Arc); + +impl Drop for LifecycleRegistration { + fn drop(&mut self) { + self.0.fetch_sub(/*val*/ 1, Relaxed); + } +} + +fn approved_response() -> ElicitationResponse { + ElicitationResponse { + action: ElicitationAction::Accept, + content: Some(json!({})), + meta: Some(json!({ "approvals_reviewer": "auto_review" })), + } +} + +fn elicitation_fixture( + approval_policy: AskForApproval, + permission_profile: PermissionProfile, + reviewer: Option>, +) -> (ElicitationRequestManager, Receiver, SendElicitation) { + let lifecycle = reviewer.as_ref().map(|reviewer| { + let active_elicitations = reviewer.active_elicitations.clone(); + ElicitationLifecycle::new(move || { + active_elicitations.fetch_add(/*val*/ 1, Relaxed); + LifecycleRegistration(active_elicitations.clone()) + }) + }); + let manager = ElicitationRequestManager::new( + approval_policy, + permission_profile, + reviewer.map(|reviewer| reviewer as Arc), + lifecycle, + ElicitationRequestRouter::default(), + ); + let (tx_event, events) = async_channel::bounded(1); + let sender = manager.make_sender("independent-mcp".to_string(), Some(tx_event)); + (manager, events, sender) +} + +async fn send_elicitation(sender: &SendElicitation, marker: Option) -> ElicitationResponse { + let elicitation = Elicitation::Mcp(ElicitRequestParams::FormElicitationParams { + meta: marker.map(|value| { + RequestMetaObject::from(Map::from_iter([(STRICT_AUTO_REVIEW_KEY.into(), value)])) + }), + message: "Review this request".to_string(), + requested_schema: ElicitationSchema::builder().build().unwrap(), + }); + sender(RequestId::Number(7), elicitation) + .await + .expect("elicitation must receive a terminal response") +} + +async fn assert_declined(marker: Value, response: Option) { + let expected_calls = usize::from(marker == Value::Bool(true)); + let reviewer = response.map(RecordingReviewer::new); + let (_, events, sender) = elicitation_fixture( + AskForApproval::Never, + PermissionProfile::Disabled, + reviewer.clone(), + ); + assert_eq!( + send_elicitation(&sender, Some(marker)).await, + strict_auto_review_decline() + ); + if let Some(reviewer) = reviewer { + assert_eq!(reviewer.calls.load(Relaxed), expected_calls); + } + assert!(events.is_empty()); +} + +#[tokio::test] +async fn strict_auto_review_respects_explicit_elicitation_denials() { + for policy in [ + AskForApproval::OnRequest, + AskForApproval::UnlessTrusted, + AskForApproval::Never, + AskForApproval::Granular(GranularApprovalConfig { + sandbox_approval: true, + rules: true, + skill_approval: true, + request_permissions: true, + mcp_elicitations: false, + }), + ] { + let explicitly_denied = matches!( + policy, + AskForApproval::Granular(config) if !config.allows_mcp_elicitations() + ); + let reviewer = RecordingReviewer::new(Ok(Some(approved_response()))); + let (manager, events, sender) = + elicitation_fixture(policy, PermissionProfile::Disabled, Some(reviewer.clone())); + assert_eq!( + send_elicitation(&sender, Some(json!(true))).await, + if explicitly_denied { + strict_auto_review_decline() + } else { + approved_response() + } + ); + if policy == AskForApproval::Never { + for (server_name, marker) in [ + ("independent-mcp", Some(json!(false))), + ("another-independent-mcp", None), + ] { + let sender = manager.make_sender(server_name.into(), /*tx_event*/ None); + assert_eq!( + send_elicitation(&sender, marker).await, + ElicitationResponse { + meta: None, + ..approved_response() + }, + ); + } + } + manager.router.set_auto_deny(/*auto_deny*/ true); + assert_eq!( + send_elicitation(&sender, Some(json!(true))).await, + ElicitationResponse { + meta: None, + ..strict_auto_review_decline() + }, + ); + assert_eq!( + ( + reviewer.calls.load(Relaxed), + reviewer.active_elicitations.load(Relaxed) + ), + (usize::from(!explicitly_denied), 0), + ); + assert!(events.is_empty(), "strict review must not emit an event"); + } +} + +#[tokio::test] +async fn strict_auto_review_fails_closed_without_a_canonical_decision() { + for marker in ["null", "\"true\"", "1", "{}", "[true]"] { + let marker = serde_json::from_str(marker).expect("valid malformed marker"); + assert_declined(marker, Some(Ok(Some(approved_response())))).await; + } + for response in [Ok(None), Err("reviewer failed")] { + assert_declined(json!(true), Some(response)).await; + } + let invalid_decisions: [fn(&mut ElicitationResponse); 6] = [ + |response| { + response.action = ElicitationAction::Decline; + response.meta = Some(json!({ "message": "Ask the user to approve this request." })); + }, + |response| response.action = ElicitationAction::Cancel, + |response| response.meta = None, + |response| response.meta = Some(json!({ "approvals_reviewer": "user" })), + |response| response.meta = Some(json!({ "approvals_reviewer": "guardian_subagent" })), + |response| response.content = Some(json!({ "approved_for_session": true })), + ]; + for make_invalid in invalid_decisions { + let mut response = approved_response(); + make_invalid(&mut response); + assert_declined(json!(true), Some(Ok(Some(response)))).await; + } + assert_declined(json!(true), /*response*/ None).await; +} diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index cc510f2c05..7c9d30dcde 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -391,7 +391,8 @@ async fn forward_events( // runtime after a refresh. let metadata = session .mcp_tool_approval_metadata(&id, &event.call_id) - .await; + .await + .map(|(_, metadata)| metadata); pending_mcp_invocations .lock() .await diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index f71cacfef3..1d776a18ae 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -211,7 +211,7 @@ pub(crate) async fn handle_mcp_tool_call( .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), }; } - sess.register_mcp_tool_approval_metadata(turn_context, &call_id, metadata.clone()) + sess.register_mcp_tool_approval_metadata(turn_context, &call_id, &invocation, metadata.clone()) .await; notify_mcp_tool_call_started( sess.as_ref(), @@ -1047,7 +1047,7 @@ impl McpToolApprovalPolicy { #[derive(Clone)] pub(crate) struct McpToolApprovalMetadata { annotations: Option, - connector_id: Option, + pub(crate) connector_id: Option, link_id: Option, connector_name: Option, connector_description: Option, @@ -1065,6 +1065,7 @@ impl Session { &self, turn_context: &TurnContext, call_id: &str, + invocation: &McpInvocation, metadata: McpToolApprovalMetadata, ) { let Some(turn_state) = self @@ -1074,17 +1075,18 @@ impl Session { else { return; }; - turn_state - .lock() - .await - .insert_mcp_tool_approval_metadata(call_id.to_string(), metadata); + turn_state.lock().await.insert_mcp_tool_approval_metadata( + call_id.to_string(), + (invocation.server == CODEX_APPS_MCP_SERVER_NAME).then(|| invocation.clone()), + metadata, + ); } pub(crate) async fn mcp_tool_approval_metadata( &self, sub_id: &str, call_id: &str, - ) -> Option { + ) -> Option<(Option, McpToolApprovalMetadata)> { let turn_state = self .input_queue .turn_state_for_sub_id(&self.active_turn, sub_id) diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 9b56d355ab..042a7014e5 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -9,6 +9,7 @@ use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_mcp::ElicitationReviewRequest; use codex_mcp::ElicitationReviewer; use codex_mcp::ElicitationReviewerHandle; +use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; use codex_protocol::capabilities::CapabilityRootLocation; use codex_protocol::capabilities::SelectedCapabilityRoot; use codex_protocol::config_types::ApprovalsReviewer; @@ -19,8 +20,10 @@ use codex_protocol::mcp_approval_meta::APPROVALS_REVIEWER_KEY as MCP_ELICITATION use codex_protocol::mcp_approval_meta::CONNECTOR_DESCRIPTION_KEY as MCP_ELICITATION_CONNECTOR_DESCRIPTION_KEY; use codex_protocol::mcp_approval_meta::CONNECTOR_ID_KEY as MCP_ELICITATION_CONNECTOR_ID_KEY; use codex_protocol::mcp_approval_meta::CONNECTOR_NAME_KEY as MCP_ELICITATION_CONNECTOR_NAME_KEY; +use codex_protocol::mcp_approval_meta::PERSIST_KEY as MCP_ELICITATION_PERSIST_KEY; use codex_protocol::mcp_approval_meta::REQUEST_TYPE_APPROVAL_REQUEST as MCP_ELICITATION_REQUEST_TYPE_APPROVAL_REQUEST; use codex_protocol::mcp_approval_meta::REQUEST_TYPE_KEY as MCP_ELICITATION_REQUEST_TYPE_KEY; +use codex_protocol::mcp_approval_meta::STRICT_AUTO_REVIEW_KEY as MCP_ELICITATION_STRICT_AUTO_REVIEW_KEY; use codex_protocol::mcp_approval_meta::TOOL_DESCRIPTION_KEY as MCP_ELICITATION_TOOL_DESCRIPTION_KEY; use codex_protocol::mcp_approval_meta::TOOL_NAME_KEY as MCP_ELICITATION_TOOL_NAME_KEY; use codex_protocol::mcp_approval_meta::TOOL_PARAMS_KEY as MCP_ELICITATION_TOOL_PARAMS_KEY; @@ -649,7 +652,7 @@ async fn review_guardian_mcp_elicitation( session: Arc, request: ElicitationReviewRequest, ) -> anyhow::Result> { - let Some((turn_context, _cancellation_token)) = + let Some((turn_context, cancellation_token)) = session.active_turn_context_and_cancellation_token().await else { return Ok(None); @@ -658,6 +661,100 @@ async fn review_guardian_mcp_elicitation( let Some(mcp_config) = session.services.mcp_runtime.current_config() else { return Ok(None); }; + + if matches!( + request + .elicitation + .meta() + .and_then(|meta| meta.get(MCP_ELICITATION_STRICT_AUTO_REVIEW_KEY)), + Some(Value::Bool(true)) + ) { + let connector_id = elicitation_connector_id(&request.elicitation); + let trusted_guardian_request = if request.server_name == CODEX_APPS_MCP_SERVER_NAME { + let Some(call_id) = request + .elicitation + .meta() + .and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY)) + .and_then(Value::as_object) + .and_then(|meta| meta.get("call_id")) + .and_then(Value::as_str) + else { + return Ok(None); + }; + let Some((Some(invocation), metadata)) = session + .mcp_tool_approval_metadata(&turn_context.sub_id, call_id) + .await + else { + return Ok(None); + }; + if invocation.server != request.server_name + || connector_id != metadata.connector_id.as_deref() + || request + .elicitation + .meta() + .and_then(|meta| metadata_str(meta, MCP_ELICITATION_TOOL_NAME_KEY)) + != Some(invocation.tool.as_str()) + { + return Ok(None); + } + Some( + crate::mcp_tool_call::build_guardian_mcp_tool_review_request( + call_id, + &invocation, + Some(&metadata), + ), + ) + } else { + None + }; + if !turn_context + .config + .features + .enabled(Feature::GuardianApproval) + || mcp_config + .config_layer_stack + .requirements() + .approvals_reviewer + .can_set(&ApprovalsReviewer::AutoReview) + .is_err() + || crate::connectors::mcp_approvals_reviewer_from_layers( + &mcp_config.config_layer_stack, + ApprovalsReviewer::AutoReview, + request.server_name.as_str(), + connector_id, + ) != ApprovalsReviewer::AutoReview + || request + .elicitation + .meta() + .is_some_and(|meta| meta.contains_key(MCP_ELICITATION_PERSIST_KEY)) + { + return Ok(None); + } + + let GuardianElicitationReview::ApprovalRequest(guardian_request) = + guardian_elicitation_review_request(&request) + else { + return Ok(None); + }; + let guardian_request = trusted_guardian_request.unwrap_or(*guardian_request); + let decision = crate::guardian::review_approval_request_with_cancel( + &session, + &turn_context, + crate::guardian::new_guardian_review_id(), + guardian_request, + /*retry_reason*/ None, + crate::guardian::GuardianReviewOptions { + plugin_attribution_override: None, + approval_request_source: codex_analytics::GuardianApprovalRequestSource::MainTurn, + external_cancel: Some(cancellation_token), + }, + ) + .await; + + return Ok(matches!(decision, ReviewDecision::Approved) + .then(|| mcp_elicitation_response_from_guardian_decision(decision))); + } + let approval_policy = mcp_config.approval_policy.value(); match approval_policy { AskForApproval::Never => { @@ -688,6 +785,7 @@ async fn review_guardian_mcp_elicitation( AskForApproval::OnRequest | AskForApproval::UnlessTrusted | AskForApproval::Granular(_) => { } } + let approvals_reviewer = crate::connectors::mcp_approvals_reviewer_from_layers( &mcp_config.config_layer_stack, mcp_config.approvals_reviewer, diff --git a/codex-rs/core/src/state/turn.rs b/codex-rs/core/src/state/turn.rs index 2a18f72d02..79c103745f 100644 --- a/codex-rs/core/src/state/turn.rs +++ b/codex-rs/core/src/state/turn.rs @@ -23,6 +23,7 @@ use crate::session::TurnInputQueue; use crate::session::turn_context::TurnContext; use crate::tasks::AnySessionTask; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::TokenUsage; @@ -88,7 +89,7 @@ pub(crate) struct TurnState { pending_request_permissions: HashMap, pending_user_input: HashMap>, pending_elicitations: HashMap<(String, RequestId), oneshot::Sender>, - mcp_tool_approval_metadata: HashMap, + mcp_tool_approval_metadata: HashMap, McpToolApprovalMetadata)>, pending_dynamic_tools: HashMap>, pub(crate) pending_input: TurnInputQueue, mailbox_delivery_phase: MailboxDeliveryPhase, @@ -183,15 +184,17 @@ impl TurnState { pub(crate) fn insert_mcp_tool_approval_metadata( &mut self, call_id: String, + invocation: Option, metadata: McpToolApprovalMetadata, ) { - self.mcp_tool_approval_metadata.insert(call_id, metadata); + self.mcp_tool_approval_metadata + .insert(call_id, (invocation, metadata)); } pub(crate) fn mcp_tool_approval_metadata( &self, call_id: &str, - ) -> Option { + ) -> Option<(Option, McpToolApprovalMetadata)> { self.mcp_tool_approval_metadata.get(call_id).cloned() } diff --git a/codex-rs/protocol/src/mcp_approval_meta.rs b/codex-rs/protocol/src/mcp_approval_meta.rs index 7a8695a9b6..8b221ae741 100644 --- a/codex-rs/protocol/src/mcp_approval_meta.rs +++ b/codex-rs/protocol/src/mcp_approval_meta.rs @@ -3,6 +3,7 @@ pub const APPROVAL_KIND_MCP_TOOL_CALL: &str = "mcp_tool_call"; pub const APPROVAL_KIND_TOOL_SUGGESTION: &str = "tool_suggestion"; pub const REQUEST_TYPE_KEY: &str = "codex_request_type"; pub const REQUEST_TYPE_APPROVAL_REQUEST: &str = "approval_request"; +pub const STRICT_AUTO_REVIEW_KEY: &str = "codex_strict_auto_review"; pub const APPROVALS_REVIEWER_KEY: &str = "approvals_reviewer"; pub const PERSIST_KEY: &str = "persist"; pub const PERSIST_SESSION: &str = "session";