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

@@ -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)

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,

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));