From 0e37d834d4f2fefea63da419bbc52055af33e888 Mon Sep 17 00:00:00 2001 From: Alex Zamoshchin Date: Tue, 1 Sep 2026 13:08:49 +0000 Subject: [PATCH] Honor explicit account selectors for Apps tool calls (#42054) ## What changed - Resolve `link_id` from tool-call arguments when Apps metadata sets `requires_explicit_link_id` to `true`. - Reject the call before approval or execution when the required selector is missing, empty, or not a string. - Preserve catalog-provided account metadata for legacy Apps tools and leave non-Apps MCP tools unchanged. ## Testing - Add unit coverage for required selectors, malformed values, legacy fallbacks, and non-Apps tools. - Add end-to-end coverage for execution, approval prompts, and rejection when an Apps call omits `link_id`. GitOrigin-RevId: 667fd04102a3934d53ded020da6d1bf68a1ca3e5 --- codex-rs/core/src/mcp_tool_call.rs | 58 ++++-- codex-rs/core/src/mcp_tool_call/account.rs | 39 ++++ codex-rs/core/src/mcp_tool_call_tests.rs | 92 +++++++++ .../core/tests/suite/mcp_turn_metadata.rs | 192 +++++++++++++++++- 4 files changed, 365 insertions(+), 16 deletions(-) create mode 100644 codex-rs/core/src/mcp_tool_call/account.rs diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index a21fa49be1..7461a01f75 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -96,8 +96,10 @@ use tracing::error; use tracing::field::Empty; use url::Url; +mod account; mod telemetry; +use account::McpToolAccountError; use telemetry::McpCallMetricOutcome; use telemetry::emit_mcp_call_metrics; use telemetry::mcp_call_metric_outcome; @@ -173,7 +175,32 @@ pub(crate) async fn handle_mcp_tool_call( .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), }; }; - let metadata = mcp_tool_metadata(&prepared_call); + let metadata = match mcp_tool_metadata( + prepared_call.tool_info(), + prepared_call.plugin_id(), + invocation.arguments.as_ref(), + ) { + Ok(metadata) => metadata, + Err(err) => { + let item_metadata = + McpToolCallItemMetadata::from_tool_metadata(&server, /*metadata*/ None); + let result = notify_mcp_tool_call_skip( + sess.as_ref(), + turn_context.as_ref(), + &call_id, + invocation, + item_metadata, + err.to_string(), + /*already_started*/ false, + ) + .await; + return HandledMcpToolCall { + result: CallToolResult::from_result(result), + tool_input: arguments_value + .unwrap_or_else(|| JsonValue::Object(serde_json::Map::new())), + }; + } + }; let item_metadata = McpToolCallItemMetadata::from_tool_metadata(&server, Some(&metadata)); let runtime_config = prepared_call.config(); let app_tool_policy = if server == CODEX_APPS_MCP_SERVER_NAME { @@ -1642,9 +1669,13 @@ pub(crate) fn build_guardian_mcp_tool_review_request( } } -fn mcp_tool_metadata(prepared_call: &PreparedMcpCall) -> McpToolApprovalMetadata { - let server = prepared_call.server_name(); - let tool_info = prepared_call.tool_info().clone(); +fn mcp_tool_metadata( + tool_info: &ToolInfo, + plugin_id: Option<&str>, + arguments: Option<&JsonValue>, +) -> Result { + let server = tool_info.server_name.as_str(); + let tool_info = tool_info.clone(); let connector_description = (server == CODEX_APPS_MCP_SERVER_NAME) .then(|| tool_info.namespace_description.clone()) .flatten(); @@ -1656,6 +1687,11 @@ fn mcp_tool_metadata(prepared_call: &PreparedMcpCall) -> McpToolApprovalMetadata .and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY)) .and_then(serde_json::Value::as_object) .cloned(); + let link_id = if server == CODEX_APPS_MCP_SERVER_NAME { + account::resolve_account(&tool_info, arguments)? + } else { + None + }; let connected_account_email = if server == CODEX_APPS_MCP_SERVER_NAME { codex_apps_meta .as_ref() @@ -1668,20 +1704,14 @@ fn mcp_tool_metadata(prepared_call: &PreparedMcpCall) -> McpToolApprovalMetadata None }; - McpToolApprovalMetadata { + Ok(McpToolApprovalMetadata { annotations: tool_info.tool.annotations, connector_id: tool_info.connector_id, - link_id: tool_info - .tool - .meta - .as_ref() - .and_then(|meta| meta.get(MCP_TOOL_LINK_ID_META_KEY)) - .and_then(serde_json::Value::as_str) - .map(str::to_string), + link_id, connector_name: tool_info.connector_name, connector_description, connected_account_email, - plugin_id: prepared_call.plugin_id().map(str::to_string), + plugin_id: plugin_id.map(str::to_string), tool_title: tool_info.tool.title, tool_description: tool_info.tool.description.map(std::borrow::Cow::into_owned), mcp_app_resource_uri: get_mcp_app_resource_uri(tool_info.tool.meta.as_deref()), @@ -1691,7 +1721,7 @@ fn mcp_tool_metadata(prepared_call: &PreparedMcpCall) -> McpToolApprovalMetadata server, &tool_info.openai_file_input_optional_fields, ), - } + }) } fn openai_file_input_optional_fields_for_server( diff --git a/codex-rs/core/src/mcp_tool_call/account.rs b/codex-rs/core/src/mcp_tool_call/account.rs new file mode 100644 index 0000000000..35d6fb98bf --- /dev/null +++ b/codex-rs/core/src/mcp_tool_call/account.rs @@ -0,0 +1,39 @@ +//! Resolves account identity before native Apps approvals. Required selectors +//! must be valid; optional catalog identities never block legacy calls. + +use super::MCP_TOOL_LINK_ID_META_KEY; +use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY; +use codex_mcp::ToolInfo; +use serde_json::Value as JsonValue; + +#[derive(Debug, PartialEq, Eq, thiserror::Error)] +pub(super) enum McpToolAccountError { + #[error("This app tool requires a non-empty string link_id argument")] + InvalidSelector, +} + +pub(super) fn resolve_account( + tool_info: &ToolInfo, + arguments: Option<&JsonValue>, +) -> Result, McpToolAccountError> { + let tool_meta = tool_info.tool.meta.as_deref(); + let requires_explicit_link_id = tool_meta + .and_then(|meta| meta.get(MCP_TOOL_CODEX_APPS_META_KEY)) + .and_then(|meta| meta.get("requires_explicit_link_id")) + .and_then(JsonValue::as_bool) + == Some(true); + let link_id = if requires_explicit_link_id { + arguments.and_then(|arguments| arguments.get(MCP_TOOL_LINK_ID_META_KEY)) + } else { + tool_meta.and_then(|meta| meta.get(MCP_TOOL_LINK_ID_META_KEY)) + } + .and_then(JsonValue::as_str) + .filter(|link_id| !link_id.trim().is_empty()) + .map(str::to_owned); + + if requires_explicit_link_id && link_id.is_none() { + Err(McpToolAccountError::InvalidSelector) + } else { + Ok(link_id) + } +} diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 735d83c29e..333e4c4fe4 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1,3 +1,4 @@ +use super::account::McpToolAccountError; use super::*; use crate::config::ConfigBuilder; use crate::config::ManagedFeatures; @@ -97,6 +98,97 @@ fn approval_config(turn_context: &TurnContext) -> codex_mcp::McpConfig { (*mcp_config_for_test(&turn_context.config)).clone() } +#[test_case::test_case(false, None, Ok(Some("default_link")); "catalog_account")] +#[test_case::test_case(true, None, Err(McpToolAccountError::InvalidSelector); "required_selector_omitted")] +#[test_case::test_case(true, Some(serde_json::json!(null)), Err(McpToolAccountError::InvalidSelector); "null_selector")] +#[test_case::test_case(true, Some(serde_json::json!(42)), Err(McpToolAccountError::InvalidSelector); "invalid_selector")] +#[test_case::test_case(true, Some(serde_json::json!(" ")), Err(McpToolAccountError::InvalidSelector); "empty_selector")] +#[test_case::test_case(true, Some(serde_json::json!(" selected_link ")), Ok(Some(" selected_link ")); "opaque_selector")] +#[test_case::test_case(false, Some(serde_json::json!("selected_link")), Ok(Some("default_link")); "unrelated_link_argument")] +fn mcp_tool_metadata_resolves_advertised_account_selector( + requires_explicit_link_id: bool, + selected_link: Option, + expected: Result, McpToolAccountError>, +) { + let tool_info = serde_json::from_value(serde_json::json!({ + "server_name": CODEX_APPS_MCP_SERVER_NAME, + "tool_name": "events/create", + "tool_namespace": "calendar", + "connector_id": "calendar", + "tool": { + "name": "calendar/events/create", + "inputSchema": {}, + "_meta": { + "link_id": "default_link", + "_codex_apps": { + "requires_explicit_link_id": requires_explicit_link_id, + }, + }, + }, + })) + .expect("tool info"); + let arguments = selected_link.map(|link_id| serde_json::json!({ "link_id": link_id })); + assert_eq!( + mcp_tool_metadata(&tool_info, /*plugin_id*/ None, arguments.as_ref()) + .map(|metadata| metadata.link_id), + expected.map(|link_id| link_id.map(str::to_owned)), + ); +} + +#[test_case::test_case(None, Ok(None); "no_tool_metadata_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({})), Ok(None); "no_apps_metadata_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "_codex_apps": {} })), Ok(None); "missing_selector_flag_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "_codex_apps": { "requires_explicit_link_id": false } })), Ok(None); "false_selector_flag_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "_codex_apps": { "requires_explicit_link_id": null } })), Ok(None); "null_selector_flag_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "_codex_apps": { "requires_explicit_link_id": "true" } })), Ok(None); "string_true_does_not_require_selector")] +#[test_case::test_case(Some(serde_json::json!({ "_codex_apps": { "requires_explicit_link_id": 1 } })), Ok(None); "numeric_one_does_not_require_selector")] +#[test_case::test_case(Some(serde_json::json!({ "_codex_apps": [] })), Ok(None); "malformed_apps_metadata_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "link_id": null })), Ok(None); "null_catalog_link_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "link_id": 42, "_codex_apps": { "requires_explicit_link_id": false } })), Ok(None); "invalid_optional_catalog_link_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "link_id": " ", "_codex_apps": { "requires_explicit_link_id": false } })), Ok(None); "empty_optional_catalog_link_uses_legacy_fallback")] +#[test_case::test_case(Some(serde_json::json!({ "link_id": "default_link" })), Ok(Some("default_link")); "legacy_catalog_link_needs_no_selector_metadata")] +fn mcp_tool_metadata_preserves_legacy_account_fallback( + meta: Option, + expected: Result, McpToolAccountError>, +) { + let mut tool = serde_json::json!({ "name": "calendar/events/create", "inputSchema": {} }); + if let Some(meta) = meta { + tool["_meta"] = meta; + } + let tool_info = serde_json::from_value(serde_json::json!({ + "server_name": CODEX_APPS_MCP_SERVER_NAME, + "tool_name": "events/create", + "tool_namespace": "calendar", + "connector_id": "calendar", + "tool": tool, + })) + .expect("tool info"); + + assert_eq!( + mcp_tool_metadata(&tool_info, /*plugin_id*/ None, /*arguments*/ None) + .map(|metadata| metadata.link_id), + expected.map(|link_id| link_id.map(str::to_owned)), + ); +} + +#[test_case::test_case(serde_json::json!({}); "no_account_metadata")] +#[test_case::test_case(serde_json::json!({ "_codex_apps": { "requires_explicit_link_id": true } }); "unrelated_apps_metadata")] +fn non_apps_tool_does_not_require_account_metadata(meta: JsonValue) { + let tool_info = serde_json::from_value(serde_json::json!({ + "server_name": "custom_server", + "tool_name": "events/create", + "tool_namespace": "calendar", + "tool": { "name": "calendar/events/create", "inputSchema": {}, "_meta": meta }, + })) + .expect("tool info"); + + assert_eq!( + mcp_tool_metadata(&tool_info, /*plugin_id*/ None, /*arguments*/ None) + .map(|metadata| metadata.link_id), + Ok(None), + ); +} + fn mcp_turn_metadata_context(turn_context: &TurnContext) -> McpTurnMetadataContext<'_> { McpTurnMetadataContext { model: turn_context.model_info().slug.as_str(), diff --git a/codex-rs/core/tests/suite/mcp_turn_metadata.rs b/codex-rs/core/tests/suite/mcp_turn_metadata.rs index d1dab757b5..a58272552c 100644 --- a/codex-rs/core/tests/suite/mcp_turn_metadata.rs +++ b/codex-rs/core/tests/suite/mcp_turn_metadata.rs @@ -22,6 +22,7 @@ use codex_protocol::protocol::ElicitationAction; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::Op; use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::protocol::TurnEnvironmentSelections; use codex_protocol::request_permissions::PermissionGrantScope; use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionsResponse; @@ -34,6 +35,7 @@ use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL; use core_test_support::apps_test_server::SEARCH_CALENDAR_LIST_TOOL; use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE; use core_test_support::apps_test_server::recorded_apps_tool_call_by_call_id; +use core_test_support::apps_test_server::recorded_apps_tool_calls; use core_test_support::apps_test_server::search_capable_apps_builder; use core_test_support::responses::assert_root_turn; use core_test_support::responses::ev_assistant_message; @@ -46,14 +48,20 @@ use core_test_support::responses::sse; use core_test_support::responses::start_mock_server; use core_test_support::skip_if_no_network; use core_test_support::test_codex::TestCodex; -use core_test_support::test_codex::local_selections; use core_test_support::test_codex::turn_permission_fields; use core_test_support::wait_for_event; use core_test_support::wait_for_event_match; use pretty_assertions::assert_eq; +use serde_json::Value; use serde_json::json; use std::collections::HashMap; use test_case::test_case; +use wiremock::Mock; +use wiremock::Request; +use wiremock::ResponseTemplate; +use wiremock::matchers::body_partial_json; +use wiremock::matchers::method; +use wiremock::matchers::path_regex; fn set_calendar_approval_mode(config: &mut Config, approval_mode: AppToolApproval) { let approval_mode = match approval_mode { @@ -119,7 +127,10 @@ async fn submit_user_turn( text_elements: Vec::new(), }]) .with_thread_settings(ThreadSettingsOverrides { - environments: Some(local_selections(test.config.cwd.clone())), + environments: Some(TurnEnvironmentSelections::new( + test.config.cwd.clone(), + vec![test.executor_environment().selection().clone()], + )), approval_policy: Some(approval_policy), sandbox_policy: Some(sandbox_policy), permission_profile, @@ -492,6 +503,183 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar Ok(()) } +const INVALID_APP_SELECTOR_ERROR: &str = + "This app tool requires a non-empty string link_id argument"; + +#[derive(Clone, Copy)] +enum MissingAppLinkOutcome { + Execute, + Prompt, + Reject(&'static str), +} + +#[test_case(Some(json!(false)), AppToolApproval::Approve, MissingAppLinkOutcome::Execute; "legacy_false_uses_connector_approval")] +#[test_case(Some(json!(false)), AppToolApproval::Prompt, MissingAppLinkOutcome::Prompt; "legacy_false_uses_connector_prompt")] +#[test_case(Some(json!(true)), AppToolApproval::Approve, MissingAppLinkOutcome::Reject(INVALID_APP_SELECTOR_ERROR); "required_selector_cannot_use_connector_approval")] +#[test_case(None, AppToolApproval::Approve, MissingAppLinkOutcome::Execute; "legacy_absence_uses_connector_approval")] +#[test_case(Some(json!("true")), AppToolApproval::Approve, MissingAppLinkOutcome::Execute; "string_true_uses_connector_approval")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apps_missing_link_respects_advertised_selector( + requires_explicit_link_id: Option, + connector_approval: AppToolApproval, + expected: MissingAppLinkOutcome, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount(&server).await?; + let call_id = "calendar-missing-link"; + let calendar_args = json!({ "title": "Lunch", "starts_at": "2026-03-10T12:00:00Z" }); + let mut required = vec!["title", "starts_at"]; + if matches!(&requires_explicit_link_id, Some(Value::Bool(true))) { + required.push("link_id"); + } + let mut apps_meta = json!({ + "resource_uri": "/calendar/link_calendar/create_event", + "contains_mcp_source": true + }); + if let Some(requires_explicit_link_id) = requires_explicit_link_id { + apps_meta["requires_explicit_link_id"] = requires_explicit_link_id; + } + let tool = json!({ + "name": "calendar_create_event", + "description": "Create a calendar event.", + "annotations": { "readOnlyHint": false }, + "inputSchema": { + "type": "object", + "properties": { + "title": { "type": "string" }, + "starts_at": { "type": "string" }, + "link_id": { "type": "string" } + }, + "required": required + }, + "_meta": { + "connector_id": "calendar", + "connector_name": "Calendar", + "_codex_apps": apps_meta + } + }); + Mock::given(method("POST")) + .and(path_regex("^/api/codex/ps/mcp/?$")) + .and(body_partial_json(json!({ "method": "tools/list" }))) + .respond_with(move |request: &Request| { + let body: Value = serde_json::from_slice(&request.body).expect("valid tools/list"); + ResponseTemplate::new(200).set_body_json(json!({ + "jsonrpc": "2.0", "id": body["id"], "result": { "tools": [tool] } + })) + }) + .with_priority(1) + .mount(&server) + .await; + let mock = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-calendar"), + ev_function_call_with_namespace( + call_id, + SEARCH_CALENDAR_NAMESPACE, + SEARCH_CALENDAR_CREATE_TOOL, + &calendar_args.to_string(), + ), + ev_completed("resp-calendar"), + ]), + sse(vec![ + ev_response_created("resp-done"), + ev_assistant_message("msg-done", "done"), + ev_completed("resp-done"), + ]), + ], + ) + .await; + let mut builder = + search_capable_apps_builder(apps_server.chatgpt_base_url).with_config(move |config| { + set_calendar_approval_mode(config, connector_approval); + config.approvals_reviewer = ApprovalsReviewer::User; + config + .features + .enable(Feature::ToolCallMcpElicitation) + .expect("test config should allow feature update"); + }); + let test = builder.build_with_auto_env(&server).await?; + submit_user_turn( + &test, + "Use [$calendar](app://calendar) to create a calendar event.", + AskForApproval::OnRequest, + PermissionProfile::Disabled, + /*collaboration_mode*/ None, + ) + .await?; + + let mut completed_calls = Vec::new(); + let mut record_call = |event: &EventMsg| { + if let EventMsg::ItemCompleted(event) = event + && let TurnItem::McpToolCall(item) = &event.item + && item.id == call_id + { + completed_calls.push((item.status, item.link_id.clone())); + } + }; + let event = wait_for_event(&test.codex, |event| { + record_call(event); + matches!( + event, + EventMsg::ElicitationRequest(_) + | EventMsg::RequestUserInput(_) + | EventMsg::TurnComplete(_) + ) + }) + .await; + assert_eq!( + matches!(&event, EventMsg::ElicitationRequest(_)), + matches!(expected, MissingAppLinkOutcome::Prompt), + ); + if let EventMsg::ElicitationRequest(request) = event { + assert_eq!(recorded_apps_tool_calls(&server).await, Vec::::new()); + test.codex + .submit(Op::ResolveElicitation { + server_name: request.server_name, + request_id: request.id, + decision: ElicitationAction::Accept, + content: None, + meta: None, + }) + .await?; + wait_for_event(&test.codex, |event| { + record_call(event); + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + } else { + assert!( + matches!(event, EventMsg::TurnComplete(_)), + "unexpected approval prompt" + ); + } + let requests = mock.requests(); + assert_eq!(requests.len(), 2); + let tool_calls = recorded_apps_tool_calls(&server).await; + match expected { + MissingAppLinkOutcome::Execute | MissingAppLinkOutcome::Prompt => { + assert_eq!(tool_calls.len(), 1); + assert_eq!(tool_calls[0]["params"]["arguments"], calendar_args); + assert_eq!(completed_calls, vec![(McpToolCallStatus::Completed, None)]); + } + MissingAppLinkOutcome::Reject(message) => { + assert_eq!(tool_calls, Vec::::new()); + assert_eq!(completed_calls, vec![(McpToolCallStatus::Failed, None)]); + assert!( + requests[1] + .function_call_output(call_id) + .to_string() + .contains(message) + ); + } + } + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apps_default_writes_prompts_for_writes_but_not_reads() -> Result<()> { skip_if_no_network!(Ok(()));