From 057d9140a8b356a4b1eb71f01c8e981f2aeef20f Mon Sep 17 00:00:00 2001 From: cazhang-oai Date: Tue, 10 Nov 2026 18:08:36 +0000 Subject: [PATCH] Honor read-only MCP tool hints in automatic approval (#60638) ## Why MCP tools marked with `readOnlyHint: true` could still require approval in automatic mode when `destructiveHint` was also true. ## What changed Give `readOnlyHint` precedence over `destructiveHint` and `openWorldHint` when deciding whether annotations require approval. Explicit prompt mode and strict automatic review continue to require their respective approval checks. ## Testing Update unit tests for conflicting annotations, prompt mode, and strict automatic review. Extend app integration coverage to verify that read-only calls proceed while writes prompt in both automatic and writes approval modes. GitOrigin-RevId: 11a930b857886bf41d09fd54c3a38607ae3d1b4e --- codex-rs/core/src/mcp_tool_call.rs | 6 +--- codex-rs/core/src/mcp_tool_call_tests.rs | 21 ++++++------ .../core/tests/suite/mcp_turn_metadata.rs | 34 ++++++++++++++++--- 3 files changed, 41 insertions(+), 20 deletions(-) diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 47f0c82015..cc05870391 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -2744,11 +2744,6 @@ fn project_mcp_tool_approval_config_folder( } fn requires_mcp_tool_approval(annotations: Option<&ToolAnnotations>) -> bool { - let destructive_hint = annotations.and_then(|annotations| annotations.destructive_hint); - if destructive_hint == Some(true) { - return true; - } - let read_only_hint = annotations .and_then(|annotations| annotations.read_only_hint) .unwrap_or(false); @@ -2756,6 +2751,7 @@ fn requires_mcp_tool_approval(annotations: Option<&ToolAnnotations>) -> bool { return false; } + let destructive_hint = annotations.and_then(|annotations| annotations.destructive_hint); destructive_hint.unwrap_or(true) || annotations .and_then(|annotations| annotations.open_world_hint) diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 0792cd279e..47b2149327 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -402,9 +402,9 @@ fn approval_required_when_read_only_false_and_open_world() { } #[test] -fn approval_required_when_destructive_even_if_read_only_true() { +fn approval_not_required_when_read_only_even_if_destructive() { let annotations = annotations(Some(true), Some(true), Some(true)); - assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true); + assert_eq!(requires_mcp_tool_approval(Some(&annotations)), false); } #[test] @@ -3259,8 +3259,13 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { ); } +#[test_case::test_case(false, AppToolApproval::Approve; "approved tool")] +#[test_case::test_case(true, AppToolApproval::Auto; "read only tool")] #[tokio::test] -async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { +async fn strict_auto_review_forces_guardian_for_mcp_policy_skip( + read_only: bool, + approval_mode: AppToolApproval, +) { let server = start_mock_server().await; let guardian_request_log = mount_sse_once( &server, @@ -3311,7 +3316,7 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { arguments: Some(serde_json::json!({ "calendar_id": "primary" })), }; let metadata = McpToolApprovalMetadata { - annotations: Some(annotations(Some(false), Some(true), Some(true))), + annotations: Some(annotations(Some(read_only), Some(true), Some(true))), connector_id: None, link_id: None, connector_name: None, @@ -3352,7 +3357,7 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() { host_cwd: &captured_mcp_config.host_cwd, }, test_environment_id(&turn_context), - McpToolApprovalPolicy::for_server(AppToolApproval::Approve), + McpToolApprovalPolicy::for_server(approval_mode), ) .await; @@ -3494,11 +3499,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval arguments: None, }; let metadata = McpToolApprovalMetadata { - annotations: Some(annotations( - Some(true), - /*destructive*/ None, - /*open_world*/ None, - )), + annotations: Some(annotations(Some(true), Some(true), Some(true))), connector_id: None, link_id: None, connector_name: None, diff --git a/codex-rs/core/tests/suite/mcp_turn_metadata.rs b/codex-rs/core/tests/suite/mcp_turn_metadata.rs index bc0e7b18ec..462a09f531 100644 --- a/codex-rs/core/tests/suite/mcp_turn_metadata.rs +++ b/codex-rs/core/tests/suite/mcp_turn_metadata.rs @@ -1383,11 +1383,35 @@ async fn apps_missing_link_respects_advertised_selector( } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn apps_default_writes_prompts_for_writes_but_not_reads() -> Result<()> { +#[test_case(AppToolApproval::Auto; "automatic approval")] +#[test_case(AppToolApproval::Writes; "writes approval")] +async fn apps_default_approval_prompts_for_writes_but_not_reads( + approval_mode: AppToolApproval, +) -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let apps_server = AppsTestServer::mount(&server).await?; + let tools = Arc::new(Mutex::new(vec![ + json!({ + "name": "calendar_list_events", + "description": "List calendar events.", + "inputSchema": {"type": "object", "properties": {}}, + "annotations": {"readOnlyHint": true, "destructiveHint": true, "openWorldHint": true}, + "_meta": {"connector_id": "calendar", "link_id": LINK_ID, "connector_name": "Calendar"} + }), + json!({ + "name": "calendar_create_event", + "description": "Create a calendar event.", + "inputSchema": { + "type": "object", + "properties": {"title": {"type": "string"}, "starts_at": {"type": "string"}}, + "required": ["title", "starts_at"] + }, + "annotations": {"readOnlyHint": false, "destructiveHint": true, "openWorldHint": true}, + "_meta": {"connector_id": "calendar", "link_id": LINK_ID, "connector_name": "Calendar"} + }), + ])); + let apps_server = AppsTestServer::mount_with_tools(&server, tools).await?; let read_call_id = "calendar-read"; let write_call_id = "calendar-write"; let list_args = serde_json::to_string(&json!({}))?; @@ -1428,14 +1452,14 @@ async fn apps_default_writes_prompts_for_writes_but_not_reads() -> Result<()> { .await; let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone()) - .with_config(|config| { + .with_config(move |config| { config .features .enable(Feature::ToolCallMcpElicitation) .expect("test config should allow feature update"); set_default_app_approval_mode_and_reviewer( config, - AppToolApproval::Writes, + approval_mode, ApprovalsReviewer::User, ); }); @@ -1480,7 +1504,7 @@ async fn apps_default_writes_prompts_for_writes_but_not_reads() -> Result<()> { }) .await; let EventMsg::McpToolCallBegin(write_begin) = next_route else { - panic!("read-only app action should not prompt in writes mode"); + panic!("read-only app action should not prompt in {approval_mode:?} mode"); }; assert_eq!(write_begin.call_id, write_call_id); assert_eq!(write_begin.read_only_hint, Some(false));