From 3a04482645b695085f4daf7c6310ab8592653fea Mon Sep 17 00:00:00 2001 From: Alex Zamoshchin Date: Tue, 1 Sep 2026 13:32:21 +0000 Subject: [PATCH] Honor app link settings for MCP tool approvals (#42056) ## What changed - Pass the resolved app `link_id` into MCP tool approval policy evaluation. - Apply link-specific `default_tools_approval_mode` and `approvals_reviewer` settings before app and global defaults, while preserving per-tool and managed policy precedence. ## Testing - Cover link-specific approval prompts, selected account links, reviewer routing, fallback behavior, and managed-policy overrides. GitOrigin-RevId: caa07ec3e73c87aad9522bd69808cd1c0b1686f5 --- codex-rs/codex-mcp/src/resource_origin.rs | 1 + codex-rs/connectors/src/app_tool_policy.rs | 9 +- .../connectors/src/app_tool_policy_tests.rs | 108 ++++++++ codex-rs/connectors/src/runtime_projection.rs | 1 + codex-rs/core/src/connectors.rs | 9 +- codex-rs/core/src/connectors_tests.rs | 97 +++++-- codex-rs/core/src/hook_runtime.rs | 1 + codex-rs/core/src/mcp_tool_call.rs | 2 + codex-rs/core/src/mcp_tool_exposure.rs | 1 + codex-rs/core/src/session/mcp.rs | 2 + .../core/tests/suite/mcp_turn_metadata.rs | 237 +++++++++++++++++- 11 files changed, 429 insertions(+), 39 deletions(-) diff --git a/codex-rs/codex-mcp/src/resource_origin.rs b/codex-rs/codex-mcp/src/resource_origin.rs index 9783185310..0337bf761d 100644 --- a/codex-rs/codex-mcp/src/resource_origin.rs +++ b/codex-rs/codex-mcp/src/resource_origin.rs @@ -286,6 +286,7 @@ impl ResourceOrigin { if !AppToolPolicyEvaluator::new(&binding.config().config_layer_stack) .policy(AppToolPolicyInput { connector_id: Some(&self.connector_id), + link_id: None, tool_name: tool_info.tool.name.as_ref(), tool_title: tool_info.tool.title.as_deref(), destructive_hint: annotations.and_then(|value| value.destructive_hint), diff --git a/codex-rs/connectors/src/app_tool_policy.rs b/codex-rs/connectors/src/app_tool_policy.rs index c7a58c9514..a32e7903e1 100644 --- a/codex-rs/connectors/src/app_tool_policy.rs +++ b/codex-rs/connectors/src/app_tool_policy.rs @@ -22,10 +22,11 @@ impl Default for AppToolPolicy { } } -/// Connector-owned metadata used to evaluate one app tool. +/// App and account metadata used to evaluate one tool. #[derive(Debug, Clone, Copy)] pub struct AppToolPolicyInput<'a> { pub connector_id: Option<&'a str>, + pub link_id: Option<&'a str>, pub tool_name: &'a str, pub tool_title: Option<&'a str>, pub destructive_hint: Option, @@ -186,6 +187,12 @@ fn app_tool_policy_from_apps_config( }); let approval = managed_approval .or_else(|| tool_config.and_then(|tool| tool.approval_mode)) + .or_else(|| { + input + .link_id + .and_then(|link_id| app?.links.as_ref()?.links.get(link_id)) + .and_then(|link| link.default_tools_approval_mode) + }) .or_else(|| app.and_then(|app| app.default_tools_approval_mode)) .or_else(|| { input diff --git a/codex-rs/connectors/src/app_tool_policy_tests.rs b/codex-rs/connectors/src/app_tool_policy_tests.rs index d795774122..8d2222fdaa 100644 --- a/codex-rs/connectors/src/app_tool_policy_tests.rs +++ b/codex-rs/connectors/src/app_tool_policy_tests.rs @@ -488,6 +488,112 @@ fn managed_approval_overrides_user_tool_approval() { ); } +#[test] +fn link_privacy_mode_overrides_app_default_and_preserves_tool_settings() { + let apps_config = serde_json::from_value(serde_json::json!({ + "calendar": { + "default_tools_approval_mode": "auto", + "tools": { "events/create": { "approval_mode": "writes" } }, + "links": { + "link_calendar": { "default_tools_approval_mode": "approve" }, + "link_other": { "default_tools_approval_mode": "prompt" }, + "link_without_privacy": {}, + }, + }, + "drive": { + "links": { "link_drive": { "default_tools_approval_mode": "prompt" } }, + }, + "without_links": { + "default_tools_approval_mode": "writes", + }, + "empty_links": { + "default_tools_approval_mode": "prompt", + "links": {}, + }, + })) + .expect("apps config"); + let evaluator = AppToolPolicyEvaluator::from_parts( + Some(apps_config), + /*requirements_apps_config*/ None, + ); + + for (link_id, approval) in [ + (Some("link_calendar"), AppToolApproval::Approve), + (Some("link_other"), AppToolApproval::Prompt), + (Some("link_without_privacy"), AppToolApproval::Auto), + (Some("link_drive"), AppToolApproval::Auto), + (None, AppToolApproval::Auto), + ] { + assert_eq!( + evaluator.policy(AppToolPolicyInput { + link_id, + ..input("events/list", /*tool_title*/ None) + }), + AppToolPolicy { + enabled: true, + approval, + } + ); + } + + assert_eq!( + evaluator.policy(AppToolPolicyInput { + link_id: Some("link_calendar"), + ..input("events/create", /*tool_title*/ None) + }), + AppToolPolicy { + enabled: true, + approval: AppToolApproval::Writes, + } + ); + + for (connector_id, approval) in [ + ("without_links", AppToolApproval::Writes), + ("empty_links", AppToolApproval::Prompt), + ] { + assert_eq!( + evaluator.policy(AppToolPolicyInput { + connector_id: Some(connector_id), + link_id: Some("link_calendar"), + ..input("events/list", /*tool_title*/ None) + }), + AppToolPolicy { + enabled: true, + approval, + } + ); + } +} + +#[test] +fn link_privacy_mode_preserves_managed_connector_requirements() { + let apps_config = serde_json::from_value(serde_json::json!({ + "calendar": { + "links": { "link_calendar": { "default_tools_approval_mode": "approve" } }, + }, + })) + .expect("apps config"); + let mut requirements = + app_tool_requirements("calendar", "events/create", AppToolApproval::Prompt); + requirements + .apps + .get_mut("calendar") + .expect("calendar requirement") + .enabled = Some(false); + let evaluator = AppToolPolicyEvaluator::from_parts(Some(apps_config), Some(&requirements)); + + assert_eq!( + evaluator.policy(AppToolPolicyInput { + link_id: Some("link_calendar"), + ..input("events/create", /*tool_title*/ None) + }), + AppToolPolicy { + enabled: false, + approval: AppToolApproval::Prompt, + } + ); +} + #[test] fn per_tool_enable_overrides_app_level_hints() { let apps_config = AppsConfigToml { @@ -705,6 +811,7 @@ fn evaluator_matches_tool_title_for_user_config() { fn input<'a>(tool_name: &'a str, tool_title: Option<&'a str>) -> AppToolPolicyInput<'a> { AppToolPolicyInput { connector_id: Some("calendar"), + link_id: None, tool_name, tool_title, destructive_hint: Some(true), @@ -795,6 +902,7 @@ fn policy_from_config_parts( }; AppToolPolicyEvaluator::new(&config_layer_stack).policy(AppToolPolicyInput { connector_id, + link_id: None, tool_name, tool_title, destructive_hint, diff --git a/codex-rs/connectors/src/runtime_projection.rs b/codex-rs/connectors/src/runtime_projection.rs index 0dbc0b4a97..5e8f100dd0 100644 --- a/codex-rs/connectors/src/runtime_projection.rs +++ b/codex-rs/connectors/src/runtime_projection.rs @@ -69,6 +69,7 @@ pub fn installed_connector_runtime<'a>( let policy_allows_tool = policy .policy(AppToolPolicyInput { connector_id: Some(connector_id), + link_id: None, tool_name: tool.tool_name, tool_title: tool.tool_title, destructive_hint: tool.destructive_hint, diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index ba6ca1a544..5929ae7d5a 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -517,6 +517,7 @@ pub(crate) fn mcp_approvals_reviewer_from_layers( model: Option<&str>, server_name: &str, connector_id: Option<&str>, + link_id: Option<&str>, ) -> ApprovalsReviewer { let requirements = config_layer_stack.requirements(); if model.is_some_and(|model| requirements.auto_review_required_for_model(model)) { @@ -525,9 +526,11 @@ pub(crate) fn mcp_approvals_reviewer_from_layers( let app_reviewer = if server_name == CODEX_APPS_MCP_SERVER_NAME { apps_config_from_layer_stack(config_layer_stack).and_then(|apps_config| { - connector_id - .and_then(|connector_id| apps_config.apps.get(connector_id)) - .and_then(|app| app.approvals_reviewer) + let app = connector_id.and_then(|connector_id| apps_config.apps.get(connector_id)); + link_id + .and_then(|link_id| app?.links.as_ref()?.links.get(link_id)) + .and_then(|link| link.approvals_reviewer) + .or_else(|| app.and_then(|app| app.approvals_reviewer)) .or_else(|| { apps_config .default diff --git a/codex-rs/core/src/connectors_tests.rs b/codex-rs/core/src/connectors_tests.rs index 34c420e2e8..6d86188450 100644 --- a/codex-rs/core/src/connectors_tests.rs +++ b/codex-rs/core/src/connectors_tests.rs @@ -304,7 +304,7 @@ fn accessible_connectors_from_mcp_tools_preserves_description() { } #[tokio::test] -async fn app_approvals_reviewer_uses_app_then_default_then_global() { +async fn app_approvals_reviewer_uses_link_then_app_then_default_then_global() { for (global, app_default, app, expected_global, expected_default, expected_app) in [ ( "user", @@ -335,6 +335,22 @@ approvals_reviewer = "{app_default}" [apps.calendar] approvals_reviewer = "{app}" + +[apps.calendar.links.link_calendar] +approvals_reviewer = "{app_default}" + +[apps.calendar.links.link_without_privacy] + +[apps.drive.links.link_drive] +approvals_reviewer = "{app_default}" + +[apps.without_links] +approvals_reviewer = "{app}" + +[apps.empty_links] +approvals_reviewer = "{app}" + +[apps.empty_links.links] "# ), ) @@ -345,23 +361,45 @@ approvals_reviewer = "{app}" .await .expect("config should build"); + for (link_id, expected) in [ + (Some("link_calendar"), expected_default), + (Some("link_without_privacy"), expected_app), + (Some("link_drive"), expected_app), + (None, expected_app), + ] { + assert_eq!( + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, + config.model.as_deref(), + CODEX_APPS_MCP_SERVER_NAME, + Some("calendar"), + link_id, + ), + expected + ); + } + for connector_id in ["without_links", "empty_links"] { + assert_eq!( + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, + config.model.as_deref(), + CODEX_APPS_MCP_SERVER_NAME, + Some(connector_id), + Some("link_calendar"), + ), + expected_app + ); + } assert_eq!( mcp_approvals_reviewer_from_layers( &config.config_layer_stack, config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, - Some("calendar") - ), - expected_app - ); - assert_eq!( - mcp_approvals_reviewer_from_layers( - &config.config_layer_stack, - config.approvals_reviewer, - config.model.as_deref(), - CODEX_APPS_MCP_SERVER_NAME, - Some("drive") + Some("drive"), + /*link_id*/ None, ), expected_default ); @@ -371,7 +409,8 @@ approvals_reviewer = "{app}" config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, - /*connector_id*/ None + /*connector_id*/ None, + /*link_id*/ None, ), expected_default ); @@ -381,7 +420,8 @@ approvals_reviewer = "{app}" config.approvals_reviewer, config.model.as_deref(), "custom_server", - Some("calendar") + Some("calendar"), + Some("link_calendar"), ), expected_global ); @@ -418,7 +458,8 @@ approvals_reviewer = "user" config.approvals_reviewer, config.model.as_deref(), CODEX_APPS_MCP_SERVER_NAME, - Some("calendar") + Some("calendar"), + /*link_id*/ None, ), ApprovalsReviewer::AutoReview ); @@ -434,6 +475,9 @@ approvals_reviewer = "auto_review" [apps.calendar] approvals_reviewer = "user" + +[apps.calendar.links.link_calendar] +approvals_reviewer = "user" "#, ) .expect("write config"); @@ -448,16 +492,19 @@ approvals_reviewer = "user" .await .expect("config should build"); - assert_eq!( - mcp_approvals_reviewer_from_layers( - &config.config_layer_stack, - config.approvals_reviewer, - config.model.as_deref(), - CODEX_APPS_MCP_SERVER_NAME, - Some("calendar") - ), - ApprovalsReviewer::AutoReview - ); + for link_id in [None, Some("link_calendar")] { + assert_eq!( + mcp_approvals_reviewer_from_layers( + &config.config_layer_stack, + config.approvals_reviewer, + config.model.as_deref(), + CODEX_APPS_MCP_SERVER_NAME, + Some("calendar"), + link_id, + ), + ApprovalsReviewer::AutoReview + ); + } } #[tokio::test] diff --git a/codex-rs/core/src/hook_runtime.rs b/codex-rs/core/src/hook_runtime.rs index 83711b97f3..7ae49b3a23 100644 --- a/codex-rs/core/src/hook_runtime.rs +++ b/codex-rs/core/src/hook_runtime.rs @@ -334,6 +334,7 @@ fn executor_hook_sources_for_step(step_context: &StepContext) -> Vec( app_tool_policy .policy(AppToolPolicyInput { connector_id: Some(connector_id), + link_id: None, tool_name: &tool.tool.name, tool_title: tool.tool.title.as_deref(), destructive_hint: annotations.and_then(|annotations| annotations.destructive_hint), diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 761183e8a0..0dffbcdb23 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -793,6 +793,7 @@ async fn review_guardian_mcp_elicitation( Some(turn_context.model_info().slug.as_str()), request.server_name.as_str(), connector_id, + /*link_id*/ None, ) != ApprovalsReviewer::AutoReview || request .elicitation @@ -877,6 +878,7 @@ async fn review_guardian_mcp_elicitation( Some(turn_context.model_info().slug.as_str()), request.server_name.as_str(), elicitation_connector_id(&request.elicitation), + /*link_id*/ None, ); if !crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) { return Ok(None); diff --git a/codex-rs/core/tests/suite/mcp_turn_metadata.rs b/codex-rs/core/tests/suite/mcp_turn_metadata.rs index a58272552c..95bb5929cc 100644 --- a/codex-rs/core/tests/suite/mcp_turn_metadata.rs +++ b/codex-rs/core/tests/suite/mcp_turn_metadata.rs @@ -31,6 +31,7 @@ use codex_protocol::request_user_input::RequestUserInputResponse; use codex_protocol::user_input::UserInput; use core_test_support::PathExt; use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::apps_test_server::LINK_ID; 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; @@ -379,10 +380,12 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request( } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -#[test_case(false; "unmanaged model")] -#[test_case(true; "protected model")] -async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guardian( +#[test_case(false, false; "unmanaged model")] +#[test_case(true, false; "protected model")] +#[test_case(false, true; "work mode link reviewer")] +async fn apps_prompt_with_auto_review_routes_actual_mcp_approval_to_guardian( protected_model: bool, + work_link_reviewer: bool, ) -> Result<()> { skip_if_no_network!(Ok(())); @@ -441,11 +444,30 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar .features .enable(Feature::ToolCallMcpElicitation) .expect("test config should allow feature update"); - set_default_app_approval_mode_and_reviewer( - config, - AppToolApproval::Prompt, - app_reviewer, - ); + if work_link_reviewer { + config.apps_mcp_product_sku = Some("tpp".to_string()); + let user_config = toml::from_str(&format!( + r#" +[apps._default] +default_tools_approval_mode = "prompt" +approvals_reviewer = "user" +[apps.calendar.links.{LINK_ID}] +default_tools_approval_mode = "prompt" +approvals_reviewer = "auto_review" +"# + )) + .expect("apps config should parse"); + config.config_layer_stack = config + .config_layer_stack + .with_user_config(&config.codex_home.join("config.toml").abs(), user_config) + .expect("apps user config should be valid"); + } else { + set_default_app_approval_mode_and_reviewer( + config, + AppToolApproval::Prompt, + app_reviewer, + ); + } }); if protected_model { builder = builder.with_model("gpt-5.4").with_cloud_config_bundle( @@ -454,7 +476,7 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar ), ); } - let test = builder.build(&server).await?; + let test = builder.build_with_auto_env(&server).await?; submit_user_turn( &test, @@ -478,7 +500,7 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar .await; assert!( matches!(route_event, EventMsg::TurnComplete(_)), - "expected apps._default auto_review to route the app approval to Guardian" + "expected the app's auto_review setting to route the approval to Guardian" ); let guardian_request = responses @@ -503,6 +525,201 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar Ok(()) } +#[test_case("approve", "prompt", None, false, true; "work_link_requires_prompt")] +#[test_case("prompt", "approve", None, false, false; "work_link_skips_prompt")] +#[test_case("approve", "approve", Some("selected_calendar_link"), true, true; "required_selector_uses_selected_link")] +#[test_case("prompt", "prompt", Some("selected_calendar_link"), false, true; "ignores_non_selector_link_argument")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apps_link_policy_controls_elicitation( + connector_mode: &str, + link_mode: &str, + selected_link_id: Option<&'static str>, + requires_explicit_link_id: bool, + expected_prompt: bool, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount(&server).await?; + let call_id = "calendar-link-policy"; + let mut calendar_args = json!({ + "title": "Lunch", + "starts_at": "2026-03-10T12:00:00Z" + }); + let mut apps_config = format!( + r#" +[apps.calendar] +default_tools_approval_mode = "{connector_mode}" + +[apps.calendar.links.{LINK_ID}] +default_tools_approval_mode = "{link_mode}" +"# + ); + if let Some(selected_link_id) = selected_link_id { + calendar_args["link_id"] = json!(selected_link_id); + let selected_link_mode = if requires_explicit_link_id { + "prompt" + } else { + "approve" + }; + apps_config.push_str(&format!( + r#" +[apps.calendar.links.{selected_link_id}] +default_tools_approval_mode = "{selected_link_mode}" +"# + )); + let mut required = vec!["title", "starts_at"]; + let link_description = if requires_explicit_link_id { + required.push("link_id"); + format!( + "Link ID for the account this call should use. Supply link_id using a link_id value below. Select only from the accounts below.\n{}", + serde_json::to_string_pretty(&json!([ + { "link_id": LINK_ID, "link_name": "Default calendar" }, + { "link_id": selected_link_id, "link_name": "Selected calendar" } + ]))? + ) + } else { + "Link identifier for the calendar resource.".to_string() + }; + 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", "description": link_description } + }, + "required": required, + "additionalProperties": false + }, + "_meta": { + "connector_id": "calendar", + "link_id": LINK_ID, + "connector_name": "Calendar", + "_codex_apps": { + "connector_id": "calendar", + "resource_uri": "connector://calendar/tools/calendar_create_event", + "contains_mcp_source": true, + "requires_explicit_link_id": requires_explicit_link_id + } + } + }); + 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("Apps tools/list should be a valid JSON-RPC request"); + ResponseTemplate::new(200).set_body_json(json!({ + "jsonrpc": "2.0", + "id": body["id"], + "result": { "tools": [tool] } + })) + }) + .with_priority(1) + .mount(&server) + .await; + } + let user_config = toml::from_str(&apps_config)?; + 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| { + config.apps_mcp_product_sku = Some("tpp".to_string()); + config.approvals_reviewer = ApprovalsReviewer::User; + config + .features + .enable(Feature::ToolCallMcpElicitation) + .expect("test config should allow feature update"); + config.config_layer_stack = config + .config_layer_stack + .with_user_config(&config.codex_home.join("config.toml").abs(), user_config) + .expect("apps user config should be valid"); + }); + 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_links = Vec::new(); + let mut record_link = |event: &EventMsg| { + if let EventMsg::ItemCompleted(event) = event + && let TurnItem::McpToolCall(item) = &event.item + && item.id == call_id + { + completed_links.push(item.link_id.clone()); + } + }; + let event = wait_for_event(&test.codex, |event| { + record_link(event); + matches!( + event, + EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_) + ) + }) + .await; + assert_eq!( + matches!(&event, EventMsg::ElicitationRequest(_)), + expected_prompt + ); + if let EventMsg::ElicitationRequest(request) = event { + 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_link(event); + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + } + assert_eq!(mock.requests().len(), 2); + assert_eq!( + recorded_apps_tool_call_by_call_id(&server, call_id).await["params"]["arguments"], + calendar_args + ); + let expected_link_id = if requires_explicit_link_id { + selected_link_id + } else { + Some(LINK_ID) + }; + assert_eq!(completed_links, vec![expected_link_id.map(str::to_string)]); + Ok(()) +} + const INVALID_APP_SELECTOR_ERROR: &str = "This app tool requires a non-empty string link_id argument";