diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index f82cc73bb5..18a4843578 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -495,7 +495,7 @@ async fn maybe_request_mcp_tool_approval( approval_mode: AppToolApproval, ) -> Option { let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref()); - let approval_required = annotations.is_some_and(requires_mcp_tool_approval); + let approval_required = requires_mcp_tool_approval(annotations); let mut monitor_reason = None; let auto_approved_by_policy = approval_mode == AppToolApproval::Approve || (approval_mode == AppToolApproval::Auto && is_full_access_mode(turn_context)); @@ -1299,12 +1299,23 @@ async fn persist_codex_app_tool_approval( .await } -fn requires_mcp_tool_approval(annotations: &ToolAnnotations) -> bool { - if annotations.destructive_hint == Some(true) { +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; } - annotations.read_only_hint == Some(false) && annotations.open_world_hint == Some(true) + let read_only_hint = annotations + .and_then(|annotations| annotations.read_only_hint) + .unwrap_or(false); + if read_only_hint { + return false; + } + + destructive_hint.unwrap_or(true) + || annotations + .and_then(|annotations| annotations.open_world_hint) + .unwrap_or(true) } async fn notify_mcp_tool_call_skip( diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 0ac37f9ce8..9d7d1c3a41 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -64,19 +64,30 @@ fn prompt_options( #[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); + assert_eq!(requires_mcp_tool_approval(Some(&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); + assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true); } #[test] fn approval_required_when_destructive_even_if_read_only_true() { let annotations = annotations(Some(true), Some(true), Some(true)); - assert_eq!(requires_mcp_tool_approval(&annotations), true); + assert_eq!(requires_mcp_tool_approval(Some(&annotations)), true); +} + +#[test] +fn approval_required_when_annotations_are_absent() { + assert_eq!(requires_mcp_tool_approval(None), true); +} + +#[test] +fn approval_not_required_when_read_only_and_other_hints_are_absent() { + let annotations = annotations(Some(true), None, None); + assert_eq!(requires_mcp_tool_approval(Some(&annotations)), false); } #[test] @@ -1067,6 +1078,75 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() { ); } +#[tokio::test] +async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() { + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/codex/safety/arc")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "outcome": "steer-model", + "short_reason": "needs approval", + "rationale": "high-risk action", + "risk_score": 96, + "risk_level": "critical", + "evidence": [{ + "message": "dangerous_tool", + "why": "high-risk action", + }], + }))) + .expect(1) + .mount(&server) + .await; + + let (session, mut turn_context) = make_session_and_context().await; + turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth( + crate::CodexAuth::create_dummy_chatgpt_auth_for_testing(), + )); + let mut config = (*turn_context.config).clone(); + config.chatgpt_base_url = server.uri(); + turn_context.config = Arc::new(config); + + let session = Arc::new(session); + let turn_context = Arc::new(turn_context); + let invocation = McpInvocation { + server: CODEX_APPS_MCP_SERVER_NAME.to_string(), + tool: "dangerous_tool".to_string(), + arguments: Some(serde_json::json!({ "id": 1 })), + }; + let metadata = McpToolApprovalMetadata { + annotations: None, + connector_id: Some("calendar".to_string()), + connector_name: Some("Calendar".to_string()), + connector_description: Some("Manage events".to_string()), + tool_title: Some("Dangerous Tool".to_string()), + tool_description: Some("Performs a risky action.".to_string()), + codex_apps_meta: None, + }; + + let decision = maybe_request_mcp_tool_approval( + &session, + &turn_context, + "call-3", + &invocation, + Some(&metadata), + AppToolApproval::Approve, + ) + .await; + + assert_eq!( + decision, + Some(McpToolApprovalDecision::BlockedBySafetyMonitor( + "Tool call was cancelled because of safety risks: high-risk action".to_string(), + )) + ); +} + #[tokio::test] async fn full_access_auto_mode_blocks_when_arc_returns_interrupt_for_model() { use wiremock::Mock; diff --git a/codex-rs/core/tests/common/apps_test_server.rs b/codex-rs/core/tests/common/apps_test_server.rs index 450a170b2a..da5499c573 100644 --- a/codex-rs/core/tests/common/apps_test_server.rs +++ b/codex-rs/core/tests/common/apps_test_server.rs @@ -185,6 +185,11 @@ impl Respond for CodexAppsJsonRpcResponder { { "name": "calendar_create_event", "description": "Create a calendar event.", + "annotations": { + "readOnlyHint": false, + "destructiveHint": false, + "openWorldHint": false + }, "inputSchema": { "type": "object", "properties": { @@ -209,6 +214,9 @@ impl Respond for CodexAppsJsonRpcResponder { { "name": "calendar_list_events", "description": "List calendar events.", + "annotations": { + "readOnlyHint": true + }, "inputSchema": { "type": "object", "properties": { @@ -241,6 +249,9 @@ impl Respond for CodexAppsJsonRpcResponder { tools.push(json!({ "name": format!("calendar_timezone_option_{index}"), "description": format!("Read timezone option {index}."), + "annotations": { + "readOnlyHint": true + }, "inputSchema": { "type": "object", "properties": { diff --git a/codex-rs/rmcp-client/src/bin/test_stdio_server.rs b/codex-rs/rmcp-client/src/bin/test_stdio_server.rs index cd08307767..d9202babae 100644 --- a/codex-rs/rmcp-client/src/bin/test_stdio_server.rs +++ b/codex-rs/rmcp-client/src/bin/test_stdio_server.rs @@ -22,6 +22,7 @@ use rmcp::model::ResourceTemplate; use rmcp::model::ServerCapabilities; use rmcp::model::ServerInfo; use rmcp::model::Tool; +use rmcp::model::ToolAnnotations; use serde::Deserialize; use serde_json::json; use tokio::task; @@ -85,11 +86,13 @@ impl TestToolServer { })) .expect("echo tool schema should deserialize"); - Tool::new( + let mut tool = Tool::new( Cow::Borrowed(name), Cow::Borrowed(description), Arc::new(schema), - ) + ); + tool.annotations = Some(ToolAnnotations::new().read_only(true)); + tool } fn image_tool() -> Tool { @@ -101,11 +104,13 @@ impl TestToolServer { })) .expect("image tool schema should deserialize"); - Tool::new( + let mut tool = Tool::new( Cow::Borrowed("image"), Cow::Borrowed("Return a single image content block."), Arc::new(schema), - ) + ); + tool.annotations = Some(ToolAnnotations::new().read_only(true)); + tool } /// Tool intended for manual testing of Codex TUI rendering for MCP image tool results. @@ -154,13 +159,15 @@ impl TestToolServer { })) .expect("image_scenario tool schema should deserialize"); - Tool::new( + let mut tool = Tool::new( Cow::Borrowed("image_scenario"), Cow::Borrowed( "Return content blocks for manual testing of MCP image rendering scenarios.", ), Arc::new(schema), - ) + ); + tool.annotations = Some(ToolAnnotations::new().read_only(true)); + tool } fn memo_resource() -> Resource { diff --git a/codex-rs/rmcp-client/src/bin/test_streamable_http_server.rs b/codex-rs/rmcp-client/src/bin/test_streamable_http_server.rs index 284e1194c3..50ea06c7fa 100644 --- a/codex-rs/rmcp-client/src/bin/test_streamable_http_server.rs +++ b/codex-rs/rmcp-client/src/bin/test_streamable_http_server.rs @@ -38,6 +38,7 @@ use rmcp::model::ResourceTemplate; use rmcp::model::ServerCapabilities; use rmcp::model::ServerInfo; use rmcp::model::Tool; +use rmcp::model::ToolAnnotations; use rmcp::transport::StreamableHttpServerConfig; use rmcp::transport::StreamableHttpService; use rmcp::transport::streamable_http_server::session::local::LocalSessionManager; @@ -84,11 +85,13 @@ impl TestToolServer { })) .expect("echo tool schema should deserialize"); - Tool::new( + let mut tool = Tool::new( Cow::Borrowed("echo"), Cow::Borrowed("Echo back the provided message and include environment data."), Arc::new(schema), - ) + ); + tool.annotations = Some(ToolAnnotations::new().read_only(true)); + tool } fn memo_resource() -> Resource {