diff --git a/codex-rs/chatgpt/src/connectors.rs b/codex-rs/chatgpt/src/connectors.rs index c1a8f65359b..83b7271583f 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 { diff --git a/codex-rs/core/src/mcp_connection_manager.rs b/codex-rs/core/src/mcp_connection_manager.rs index 15305f4b0c3..1b5048ab0c5 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()) } diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 6066c1512e4..6425392fb51 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,236 @@ 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(&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, + is_secret: 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: "Decline this tool call and continue.".to_string(), + }, + RequestUserInputQuestionOption { + label: MCP_TOOL_APPROVAL_CANCEL.to_string(), + description: "Cancel this tool call".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()); + let Some(answers) = answers else { + return McpToolApprovalDecision::Cancel; + }; + if answers + .iter() + .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT) + { + McpToolApprovalDecision::Accept + } else if answers + .iter() + .any(|answer| answer == MCP_TOOL_APPROVAL_CANCEL) + { + McpToolApprovalDecision::Cancel + } else { + McpToolApprovalDecision::Decline + } +} + +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( + 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(&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(&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(&annotations), false); + } +} diff --git a/codex-rs/core/src/tools/handlers/mcp.rs b/codex-rs/core/src/tools/handlers/mcp.rs index 9798fb82414..5138b3cd0d1 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,