From d44d6cc6ead92d3652459122b1a82a1965bff0af Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 29 Jan 2026 19:13:43 -0800 Subject: [PATCH 1/7] update --- codex-rs/core/src/mcp_tool_call.rs | 325 +++++++++++++++++++++++- codex-rs/core/src/tools/handlers/mcp.rs | 3 +- 2 files changed, 324 insertions(+), 4 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 6066c1512e40..baa17a944bee 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -1,20 +1,30 @@ +use std::time::Duration; use std::time::Instant; use tracing::error; use crate::codex::Session; use crate::codex::TurnContext; +use crate::mcp::CODEX_APPS_MCP_SERVER_NAME; use crate::protocol::EventMsg; use crate::protocol::McpInvocation; use crate::protocol::McpToolCallBeginEvent; use crate::protocol::McpToolCallEndEvent; use codex_protocol::models::FunctionCallOutputPayload; use codex_protocol::models::ResponseInputItem; +use codex_protocol::protocol::AskForApproval; +use codex_protocol::protocol::SandboxPolicy; +use codex_protocol::request_user_input::RequestUserInputArgs; +use codex_protocol::request_user_input::RequestUserInputQuestion; +use codex_protocol::request_user_input::RequestUserInputQuestionOption; +use codex_protocol::request_user_input::RequestUserInputResponse; +use mcp_types::ToolAnnotations; +use std::sync::Arc; /// Handles the specified tool call dispatches the appropriate /// `McpToolCallBegin` and `McpToolCallEnd` events to the `Session`. pub(crate) async fn handle_mcp_tool_call( - sess: &Session, + sess: Arc, turn_context: &TurnContext, call_id: String, server: String, @@ -48,11 +58,79 @@ pub(crate) async fn handle_mcp_tool_call( arguments: arguments_value.clone(), }; + if let Some(decision) = + maybe_request_mcp_tool_approval(sess.as_ref(), turn_context, &call_id, &server, &tool_name) + .await + { + let result = match decision { + McpToolApprovalDecision::Accept => { + let tool_call_begin_event = EventMsg::McpToolCallBegin(McpToolCallBeginEvent { + call_id: call_id.clone(), + invocation: invocation.clone(), + }); + notify_mcp_tool_call_event(sess.as_ref(), turn_context, tool_call_begin_event) + .await; + + let start = Instant::now(); + let result = sess + .call_tool(&server, &tool_name, arguments_value.clone()) + .await + .map_err(|e| format!("tool call error: {e:?}")); + if let Err(e) = &result { + tracing::warn!("MCP tool call error: {e:?}"); + } + let tool_call_end_event = EventMsg::McpToolCallEnd(McpToolCallEndEvent { + call_id: call_id.clone(), + invocation, + duration: start.elapsed(), + result: result.clone(), + }); + notify_mcp_tool_call_event( + sess.as_ref(), + turn_context, + tool_call_end_event.clone(), + ) + .await; + result + } + McpToolApprovalDecision::Decline => { + let message = "user rejected MCP tool call".to_string(); + notify_mcp_tool_call_skip( + sess.as_ref(), + turn_context, + &call_id, + invocation, + message, + ) + .await + } + McpToolApprovalDecision::Cancel => { + let message = "user cancelled MCP tool call".to_string(); + notify_mcp_tool_call_skip( + sess.as_ref(), + turn_context, + &call_id, + invocation, + message, + ) + .await + } + }; + + let status = if result.is_ok() { "ok" } else { "error" }; + turn_context + .client + .get_otel_manager() + .counter("codex.mcp.call", 1, &[("status", status)]); + + return ResponseInputItem::McpToolCallOutput { call_id, result }; + } + let tool_call_begin_event = EventMsg::McpToolCallBegin(McpToolCallBeginEvent { call_id: call_id.clone(), invocation: invocation.clone(), }); - notify_mcp_tool_call_event(sess, turn_context, tool_call_begin_event).await; + notify_mcp_tool_call_event(sess.as_ref(), turn_context, tool_call_begin_event).await; let start = Instant::now(); // Perform the tool call. @@ -70,7 +148,7 @@ pub(crate) async fn handle_mcp_tool_call( result: result.clone(), }); - notify_mcp_tool_call_event(sess, turn_context, tool_call_end_event.clone()).await; + notify_mcp_tool_call_event(sess.as_ref(), turn_context, tool_call_end_event.clone()).await; let status = if result.is_ok() { "ok" } else { "error" }; turn_context @@ -84,3 +162,244 @@ pub(crate) async fn handle_mcp_tool_call( async fn notify_mcp_tool_call_event(sess: &Session, turn_context: &TurnContext, event: EventMsg) { sess.send_event(turn_context, event).await; } + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum McpToolApprovalDecision { + Accept, + Decline, + Cancel, +} + +struct McpToolApprovalMetadata { + annotations: ToolAnnotations, + connector_name: Option, + tool_title: Option, +} + +const MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX: &str = "mcp_tool_call_approval"; +const MCP_TOOL_APPROVAL_ACCEPT: &str = "Accept"; +const MCP_TOOL_APPROVAL_DECLINE: &str = "Decline"; +const MCP_TOOL_APPROVAL_CANCEL: &str = "Cancel"; + +async fn maybe_request_mcp_tool_approval( + sess: &Session, + turn_context: &TurnContext, + call_id: &str, + server: &str, + tool_name: &str, +) -> Option { + if is_full_access_mode(turn_context) { + return None; + } + if server != CODEX_APPS_MCP_SERVER_NAME { + return None; + } + + let metadata = lookup_mcp_tool_metadata(sess, server, tool_name).await?; + if !requires_mcp_tool_approval(Some(&metadata.annotations)) { + return None; + } + + let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}"); + let question = build_mcp_tool_approval_question( + question_id.clone(), + tool_name, + metadata.tool_title.as_deref(), + metadata.connector_name.as_deref(), + &metadata.annotations, + ); + let args = RequestUserInputArgs { + questions: vec![question], + }; + let response = sess + .request_user_input(turn_context, call_id.to_string(), args) + .await; + Some(parse_mcp_tool_approval_response(response, &question_id)) +} + +fn is_full_access_mode(turn_context: &TurnContext) -> bool { + matches!(turn_context.approval_policy, AskForApproval::Never) + && matches!( + turn_context.sandbox_policy, + SandboxPolicy::DangerFullAccess | SandboxPolicy::ExternalSandbox { .. } + ) +} + +async fn lookup_mcp_tool_metadata( + sess: &Session, + server: &str, + tool_name: &str, +) -> Option { + let tools = sess + .services + .mcp_connection_manager + .read() + .await + .list_all_tools() + .await; + + tools.into_values().find_map(|tool_info| { + if tool_info.server_name == server && tool_info.tool_name == tool_name { + tool_info + .tool + .annotations + .map(|annotations| McpToolApprovalMetadata { + annotations, + connector_name: tool_info.connector_name, + tool_title: tool_info.tool.title, + }) + } else { + None + } + }) +} + +fn build_mcp_tool_approval_question( + question_id: String, + tool_name: &str, + tool_title: Option<&str>, + connector_name: Option<&str>, + annotations: &ToolAnnotations, +) -> RequestUserInputQuestion { + let destructive = annotations.destructive_hint == Some(true); + let open_world = annotations.open_world_hint == Some(true); + let reason = match (destructive, open_world) { + (true, true) => "may modify data and access external systems", + (true, false) => "may modify or delete data", + (false, true) => "may access external systems", + (false, false) => "may have side effects", + }; + + let tool_label = tool_title.unwrap_or(tool_name); + let app_label = connector_name + .map(|name| format!("The {name} app")) + .unwrap_or_else(|| "This app".to_string()); + let question = format!( + "{app_label} wants to run the tool \"{tool_label}\", which {reason}. Allow this action?" + ); + + RequestUserInputQuestion { + id: question_id, + header: "Approve app tool call?".to_string(), + question, + is_other: false, + options: Some(vec![ + RequestUserInputQuestionOption { + label: MCP_TOOL_APPROVAL_ACCEPT.to_string(), + description: "Run the tool and continue.".to_string(), + }, + RequestUserInputQuestionOption { + label: MCP_TOOL_APPROVAL_DECLINE.to_string(), + description: "Skip this tool call and continue.".to_string(), + }, + RequestUserInputQuestionOption { + label: MCP_TOOL_APPROVAL_CANCEL.to_string(), + description: "Stop this turn.".to_string(), + }, + ]), + } +} + +fn parse_mcp_tool_approval_response( + response: Option, + question_id: &str, +) -> McpToolApprovalDecision { + let Some(response) = response else { + return McpToolApprovalDecision::Cancel; + }; + let answers = response + .answers + .get(question_id) + .map(|answer| answer.answers.as_slice()); + if answers.is_some_and(|answers| { + answers + .iter() + .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT) + }) { + McpToolApprovalDecision::Accept + } else if answers.is_some_and(|answers| { + answers + .iter() + .any(|answer| answer == MCP_TOOL_APPROVAL_CANCEL) + }) { + McpToolApprovalDecision::Cancel + } else if answers.is_none() { + McpToolApprovalDecision::Cancel + } else { + McpToolApprovalDecision::Decline + } +} + +fn requires_mcp_tool_approval(annotations: Option<&ToolAnnotations>) -> bool { + annotations.is_some_and(|annotations| { + annotations.read_only_hint == Some(false) + && (annotations.destructive_hint == Some(true) + || annotations.open_world_hint == Some(true)) + }) +} + +async fn notify_mcp_tool_call_skip( + sess: &Session, + turn_context: &TurnContext, + call_id: &str, + invocation: McpInvocation, + message: String, +) -> Result { + let tool_call_begin_event = EventMsg::McpToolCallBegin(McpToolCallBeginEvent { + call_id: call_id.to_string(), + invocation: invocation.clone(), + }); + notify_mcp_tool_call_event(sess, turn_context, tool_call_begin_event).await; + + let tool_call_end_event = EventMsg::McpToolCallEnd(McpToolCallEndEvent { + call_id: call_id.to_string(), + invocation, + duration: Duration::ZERO, + result: Err(message.clone()), + }); + notify_mcp_tool_call_event(sess, turn_context, tool_call_end_event).await; + Err(message) +} + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + + fn annotations( + read_only: Option, + destructive: Option, + open_world: Option, + ) -> ToolAnnotations { + ToolAnnotations { + destructive_hint: destructive, + idempotent_hint: None, + open_world_hint: open_world, + read_only_hint: read_only, + title: None, + } + } + + #[test] + fn approval_required_when_read_only_false_and_destructive() { + let annotations = annotations(Some(false), Some(true), None); + assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true); + } + + #[test] + fn approval_required_when_read_only_false_and_open_world() { + let annotations = annotations(Some(false), None, Some(true)); + assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true); + } + + #[test] + fn approval_not_required_when_read_only_true() { + let annotations = annotations(Some(true), Some(true), Some(true)); + assert_eq!(requires_mcp_tool_approval(Some(&annotations)), false); + } + + #[test] + fn approval_not_required_when_no_annotations() { + assert_eq!(requires_mcp_tool_approval(None), false); + } +} diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 9798fb82414b..5138b3cd0d16 100644 --- a/codex-rs/core/src/tools/handlers/mcp.rs +++ b/codex-rs/core/src/tools/handlers/mcp.rs @@ -1,4 +1,5 @@ use async_trait::async_trait; +use std::sync::Arc; use crate::function_tool::FunctionCallError; use crate::mcp_tool_call::handle_mcp_tool_call; @@ -42,7 +43,7 @@ impl ToolHandler for McpHandler { let arguments_str = raw_arguments; let response = handle_mcp_tool_call( - session.as_ref(), + Arc::clone(&session), turn.as_ref(), call_id.clone(), server, From ab5667421e5c1852b17a6c1cf06f0485f2f851ee Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 29 Jan 2026 19:18:53 -0800 Subject: [PATCH 2/7] update --- codex-rs/core/src/mcp_connection_manager.rs | 49 ++++++++++++++++++--- 1 file changed, 43 insertions(+), 6 deletions(-) diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 15305f4b0c3d..1b5048ab0c59 100644 --- a/codex-rs/core/src/mcp_connection_manager.rs +++ b/codex-rs/core/src/mcp_connection_manager.rs @@ -769,6 +769,32 @@ fn filter_tools(tools: Vec, filter: ToolFilter) -> Vec { .collect() } +fn normalize_codex_apps_tool_title( + server_name: &str, + connector_name: Option<&str>, + value: &str, +) -> String { + if server_name != CODEX_APPS_MCP_SERVER_NAME { + return value.to_string(); + } + + let Some(connector_name) = connector_name + .map(str::trim) + .filter(|name| !name.is_empty()) + else { + return value.to_string(); + }; + + let prefix = format!("{connector_name}_"); + if let Some(stripped) = value.strip_prefix(&prefix) + && !stripped.is_empty() + { + return stripped.to_string(); + } + + value.to_string() +} + fn resolve_bearer_token( server_name: &str, bearer_token_env_var: Option<&str>, @@ -926,12 +952,23 @@ async fn list_tools_for_client( Ok(resp .tools .into_iter() - .map(|tool| ToolInfo { - server_name: server_name.to_owned(), - tool_name: tool.tool.name.clone(), - tool: tool.tool, - connector_id: tool.connector_id, - connector_name: tool.connector_name, + .map(|tool| { + let connector_name = tool.connector_name; + let mut tool_def = tool.tool; + if let Some(title) = tool_def.title.as_deref() { + let normalized_title = + normalize_codex_apps_tool_title(server_name, connector_name.as_deref(), title); + if tool_def.title.as_deref() != Some(normalized_title.as_str()) { + tool_def.title = Some(normalized_title); + } + } + ToolInfo { + server_name: server_name.to_owned(), + tool_name: tool_def.name.clone(), + tool: tool_def, + connector_id: tool.connector_id, + connector_name, + } }) .collect()) } From 59d209a8d55f8ec3c85e34c8646bd1e010d976b0 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 29 Jan 2026 19:31:55 -0800 Subject: [PATCH 3/7] update --- codex-rs/core/src/mcp_tool_call.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index baa17a944bee..e02c6d3ce6dc 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -283,6 +283,7 @@ fn build_mcp_tool_approval_question( header: "Approve app tool call?".to_string(), question, is_other: false, + is_secret: false, options: Some(vec![ RequestUserInputQuestionOption { label: MCP_TOOL_APPROVAL_ACCEPT.to_string(), From 9af43fafe4abbd8278e9aaf307bf2fcaf0b9d58b Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 29 Jan 2026 19:32:47 -0800 Subject: [PATCH 4/7] update --- codex-rs/core/src/mcp_tool_call.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index e02c6d3ce6dc..5466b94c42f9 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -291,11 +291,11 @@ fn build_mcp_tool_approval_question( }, RequestUserInputQuestionOption { label: MCP_TOOL_APPROVAL_DECLINE.to_string(), - description: "Skip this tool call and continue.".to_string(), + description: "Decline this tool call and continue.".to_string(), }, RequestUserInputQuestionOption { label: MCP_TOOL_APPROVAL_CANCEL.to_string(), - description: "Stop this turn.".to_string(), + description: "Cancel this tool call".to_string(), }, ]), } From 91dd39236be2f314ee09f96d47226924987695e3 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 29 Jan 2026 19:47:40 -0800 Subject: [PATCH 5/7] update --- codex-rs/core/src/mcp_tool_call.rs | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 5466b94c42f9..28b0d695139f 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -312,19 +312,18 @@ fn parse_mcp_tool_approval_response( .answers .get(question_id) .map(|answer| answer.answers.as_slice()); - if answers.is_some_and(|answers| { - answers - .iter() - .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT) - }) { + let Some(answers) = answers else { + return McpToolApprovalDecision::Cancel; + }; + if answers + .iter() + .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT) + { McpToolApprovalDecision::Accept - } else if answers.is_some_and(|answers| { - answers - .iter() - .any(|answer| answer == MCP_TOOL_APPROVAL_CANCEL) - }) { - McpToolApprovalDecision::Cancel - } else if answers.is_none() { + } else if answers + .iter() + .any(|answer| answer == MCP_TOOL_APPROVAL_CANCEL) + { McpToolApprovalDecision::Cancel } else { McpToolApprovalDecision::Decline From 06f36928952030ebe509c696b4a26b0fa2a410b2 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 29 Jan 2026 20:02:54 -0800 Subject: [PATCH 6/7] update --- codex-rs/chatgpt/src/connectors.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/codex-rs/chatgpt/src/connectors.rs b/codex-rs/chatgpt/src/connectors.rs index c1a8f65359be..83b7271583f2 100644 --- a/codex-rs/chatgpt/src/connectors.rs +++ b/codex-rs/chatgpt/src/connectors.rs @@ -221,7 +221,11 @@ fn normalize_connector_value(value: Option<&str>) -> Option { } const ALLOWED_APPS_SDK_APPS: &[&str] = &["asdk_app_69781557cc1481919cf5e9824fa2e792"]; -const DISALLOWED_CONNECTOR_IDS: &[&str] = &["asdk_app_6938a94a61d881918ef32cb999ff937c"]; +const DISALLOWED_CONNECTOR_IDS: &[&str] = &[ + "asdk_app_6938a94a61d881918ef32cb999ff937c", + "connector_2b0a9009c9c64bf9933a3dae3f2b1254", + "connector_68de829bf7648191acd70a907364c67c", +]; const DISALLOWED_CONNECTOR_PREFIX: &str = "connector_openai_"; fn filter_disallowed_connectors(connectors: Vec) -> Vec { From 2eb97d1671b5fb5fa2cadc77400a776800b14999 Mon Sep 17 00:00:00 2001 From: Matthew Zeng Date: Thu, 29 Jan 2026 22:02:46 -0800 Subject: [PATCH 7/7] update --- codex-rs/core/src/mcp_tool_call.rs | 22 +++++++--------------- 1 file changed, 7 insertions(+), 15 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 28b0d695139f..6425392fb514 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -196,7 +196,7 @@ async fn maybe_request_mcp_tool_approval( } let metadata = lookup_mcp_tool_metadata(sess, server, tool_name).await?; - if !requires_mcp_tool_approval(Some(&metadata.annotations)) { + if !requires_mcp_tool_approval(&metadata.annotations) { return None; } @@ -330,12 +330,9 @@ fn parse_mcp_tool_approval_response( } } -fn requires_mcp_tool_approval(annotations: Option<&ToolAnnotations>) -> bool { - annotations.is_some_and(|annotations| { - annotations.read_only_hint == Some(false) - && (annotations.destructive_hint == Some(true) - || annotations.open_world_hint == Some(true)) - }) +fn requires_mcp_tool_approval(annotations: &ToolAnnotations) -> bool { + annotations.read_only_hint == Some(false) + && (annotations.destructive_hint == Some(true) || annotations.open_world_hint == Some(true)) } async fn notify_mcp_tool_call_skip( @@ -383,23 +380,18 @@ mod tests { #[test] fn approval_required_when_read_only_false_and_destructive() { let annotations = annotations(Some(false), Some(true), None); - assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true); + assert_eq!(requires_mcp_tool_approval(&annotations), true); } #[test] fn approval_required_when_read_only_false_and_open_world() { let annotations = annotations(Some(false), None, Some(true)); - assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true); + assert_eq!(requires_mcp_tool_approval(&annotations), true); } #[test] fn approval_not_required_when_read_only_true() { let annotations = annotations(Some(true), Some(true), Some(true)); - assert_eq!(requires_mcp_tool_approval(Some(&annotations)), false); - } - - #[test] - fn approval_not_required_when_no_annotations() { - assert_eq!(requires_mcp_tool_approval(None), false); + assert_eq!(requires_mcp_tool_approval(&annotations), false); } }