diff --git a/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json index c47cf09f2dba..6ad421473be6 100644 --- a/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/ApplyPatchApprovalResponse.json @@ -65,6 +65,13 @@ ], "type": "string" }, + { + "description": "User has approved this MCP tool call and wants to amend its policy so matching future calls are automatically approved across sessions.", + "enum": [ + "approved_mcp_policy_amendment" + ], + "type": "string" + }, { "additionalProperties": false, "description": "User chose to persist a network policy rule (allow/deny) for future requests to the same host.", diff --git a/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json b/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json index 7a78661926bf..22f37f3a1a72 100644 --- a/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json +++ b/codex-rs/app-server-protocol/schema/json/ExecCommandApprovalResponse.json @@ -65,6 +65,13 @@ ], "type": "string" }, + { + "description": "User has approved this MCP tool call and wants to amend its policy so matching future calls are automatically approved across sessions.", + "enum": [ + "approved_mcp_policy_amendment" + ], + "type": "string" + }, { "additionalProperties": false, "description": "User chose to persist a network policy rule (allow/deny) for future requests to the same host.", diff --git a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json index 603cc2785ce6..6ee99e1a2abd 100644 --- a/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json +++ b/codex-rs/app-server-protocol/schema/json/codex_app_server_protocol.schemas.json @@ -4258,6 +4258,13 @@ ], "type": "string" }, + { + "description": "User has approved this MCP tool call and wants to amend its policy so matching future calls are automatically approved across sessions.", + "enum": [ + "approved_mcp_policy_amendment" + ], + "type": "string" + }, { "additionalProperties": false, "description": "User chose to persist a network policy rule (allow/deny) for future requests to the same host.", diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst index 5fecb1672803..72c9643dda4a 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-experimental.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst index 861b56743eb9..15cac3939477 100644 Binary files a/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst and b/codex-rs/app-server-protocol/schema/precomputed/app-server-exports-stable.json.zst differ diff --git a/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts b/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts index 22c09a24e2b2..e2a499fbd64d 100644 --- a/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts +++ b/codex-rs/app-server-protocol/schema/typescript/ReviewDecision.ts @@ -7,4 +7,4 @@ import type { NetworkPolicyAmendment } from "./NetworkPolicyAmendment"; /** * User's decision in response to an ExecApprovalRequest. */ -export type ReviewDecision = "approved" | { "approved_execpolicy_amendment": { proposed_execpolicy_amendment: ExecPolicyAmendment, } } | "approved_for_session" | { "network_policy_amendment": { network_policy_amendment: NetworkPolicyAmendment, } } | { "denied": { rejection: string, } } | "timed_out" | "abort"; +export type ReviewDecision = "approved" | { "approved_execpolicy_amendment": { proposed_execpolicy_amendment: ExecPolicyAmendment, } } | "approved_for_session" | "approved_mcp_policy_amendment" | { "network_policy_amendment": { network_policy_amendment: NetworkPolicyAmendment, } } | { "denied": { rejection: string, } } | "timed_out" | "abort"; diff --git a/codex-rs/app-server-protocol/src/protocol/v2/item.rs b/codex-rs/app-server-protocol/src/protocol/v2/item.rs index 5c9afedde0f4..dcfe928508e8 100644 --- a/codex-rs/app-server-protocol/src/protocol/v2/item.rs +++ b/codex-rs/app-server-protocol/src/protocol/v2/item.rs @@ -82,6 +82,9 @@ impl From for CommandExecutionApprovalDecision { fn from(value: CoreReviewDecision) -> Self { match value { CoreReviewDecision::Approved => Self::Accept, + // MCP approvals are handled through elicitations, so an MCP policy amendment should + // never appear in a command execution approval. To be cautious here, we fail closed. + CoreReviewDecision::ApprovedMcpPolicyAmendment => Self::Decline, CoreReviewDecision::ApprovedExecpolicyAmendment { proposed_execpolicy_amendment, } => Self::AcceptWithExecpolicyAmendment { diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 6fb3c56c4baa..5377f93da4af 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -808,6 +808,7 @@ async fn maybe_auto_review_mcp_request_user_input( .map(|option| option.label.clone()) .unwrap_or_else(|| MCP_TOOL_APPROVAL_ACCEPT.to_string()), ReviewDecision::Approved + | ReviewDecision::ApprovedMcpPolicyAmendment | ReviewDecision::ApprovedExecpolicyAmendment { .. } | ReviewDecision::NetworkPolicyAmendment { .. } => MCP_TOOL_APPROVAL_ACCEPT.to_string(), ReviewDecision::Denied { .. } | ReviewDecision::TimedOut | ReviewDecision::Abort => { diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 02a63adf37ec..65b79013f535 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -240,9 +240,11 @@ pub(crate) async fn handle_mcp_tool_call( .await { let result = match decision { - decision @ (McpToolApprovalDecision::Accept - | McpToolApprovalDecision::AcceptForSession - | McpToolApprovalDecision::AcceptAndRemember) => { + decision @ (ReviewDecision::Approved + | ReviewDecision::ApprovedForSession + | ReviewDecision::ApprovedMcpPolicyAmendment + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::NetworkPolicyAmendment { .. }) => { return handle_approved_mcp_tool_call( &sess, step_context.as_ref(), @@ -258,20 +260,31 @@ pub(crate) async fn handle_mcp_tool_call( ) .await; } - McpToolApprovalDecision::Decline { message } => { - let message = message.unwrap_or_else(|| "user rejected MCP tool call".to_string()); + ReviewDecision::Denied { rejection } => { notify_mcp_tool_call_skip( sess.as_ref(), turn_context.as_ref(), &call_id, invocation, item_metadata.clone(), - message, + rejection, + /*already_started*/ true, + ) + .await + } + ReviewDecision::TimedOut => { + notify_mcp_tool_call_skip( + sess.as_ref(), + turn_context.as_ref(), + &call_id, + invocation, + item_metadata.clone(), + crate::guardian::guardian_timeout_message(), /*already_started*/ true, ) .await } - McpToolApprovalDecision::Cancel => { + ReviewDecision::Abort => { let message = "user cancelled MCP tool call".to_string(); notify_mcp_tool_call_skip( sess.as_ref(), @@ -1005,15 +1018,6 @@ async fn maybe_track_codex_app_used( ); } -#[derive(Debug, Clone, PartialEq, Eq)] -enum McpToolApprovalDecision { - Accept, - AcceptForSession, - AcceptAndRemember, - Decline { message: Option }, - Cancel, -} - #[derive(Clone, Copy)] struct McpToolApprovalPolicy { mode: AppToolApproval, @@ -1023,7 +1027,7 @@ struct McpToolApprovalPolicy { enum McpToolApprovalApplication { NotRequired, Apply { - decision: McpToolApprovalDecision, + decision: ReviewDecision, policy: McpToolApprovalPolicy, }, } @@ -1288,7 +1292,7 @@ async fn maybe_request_mcp_tool_approval( metadata: &McpToolApprovalMetadata, config: &codex_mcp::McpConfig, policy: McpToolApprovalPolicy, -) -> Option { +) -> Option { let turn_context = &step_context.turn; let approvals_reviewer = connectors::mcp_approvals_reviewer_from_layers( &config.config_layer_stack, @@ -1322,7 +1326,7 @@ async fn maybe_request_mcp_tool_approval( if let Some(key) = session_approval_key.as_ref() && mcp_tool_approval_is_remembered(sess, key).await { - return Some(McpToolApprovalDecision::Accept); + return Some(ReviewDecision::Approved); } match run_permission_request_hooks( @@ -1340,12 +1344,10 @@ async fn maybe_request_mcp_tool_approval( .await { Some(PermissionRequestDecision::Allow) => { - return Some(McpToolApprovalDecision::Accept); + return Some(ReviewDecision::Approved); } Some(PermissionRequestDecision::Deny { message }) => { - return Some(McpToolApprovalDecision::Decline { - message: Some(message), - }); + return Some(ReviewDecision::denied(message)); } None => {} } @@ -1369,8 +1371,10 @@ async fn maybe_request_mcp_tool_approval( Default::default(), ) .await; - let decision = mcp_tool_approval_decision_from_guardian(decision); - return Some(decision); + return Some(match decision { + ReviewDecision::Abort => ReviewDecision::denied("user rejected MCP tool call"), + decision => decision, + }); } let prompt_options = mcp_tool_approval_prompt_options( @@ -1519,22 +1523,6 @@ pub(crate) fn build_guardian_mcp_tool_review_request( } } -fn mcp_tool_approval_decision_from_guardian(decision: ReviewDecision) -> McpToolApprovalDecision { - match decision { - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept, - ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession, - ReviewDecision::Denied { rejection } => McpToolApprovalDecision::Decline { - message: Some(rejection), - }, - ReviewDecision::TimedOut => McpToolApprovalDecision::Decline { - message: Some(crate::guardian::guardian_timeout_message()), - }, - ReviewDecision::Abort => McpToolApprovalDecision::Decline { message: None }, - } -} - fn mcp_tool_metadata(prepared_call: &PreparedMcpCall) -> McpToolApprovalMetadata { let server = prepared_call.server_name(); let tool_info = prepared_call.tool_info().clone(); @@ -1824,9 +1812,9 @@ fn build_mcp_tool_approval_display_params( fn parse_mcp_tool_approval_elicitation_response( response: Option, question_id: &str, -) -> McpToolApprovalDecision { +) -> ReviewDecision { let Some(response) = response else { - return McpToolApprovalDecision::Cancel; + return ReviewDecision::Abort; }; match response.action { ElicitationAction::Accept => { @@ -1838,10 +1826,10 @@ fn parse_mcp_tool_approval_elicitation_response( .and_then(serde_json::Value::as_str) { Some(MCP_TOOL_APPROVAL_PERSIST_SESSION) => { - return McpToolApprovalDecision::AcceptForSession; + return ReviewDecision::ApprovedForSession; } Some(MCP_TOOL_APPROVAL_PERSIST_ALWAYS) => { - return McpToolApprovalDecision::AcceptAndRemember; + return ReviewDecision::ApprovedMcpPolicyAmendment; } _ => {} } @@ -1850,13 +1838,13 @@ fn parse_mcp_tool_approval_elicitation_response( request_user_input_response_from_elicitation_content(response.content), question_id, ) { - McpToolApprovalDecision::Cancel => McpToolApprovalDecision::Accept, + ReviewDecision::Abort => ReviewDecision::Approved, decision => decision, } } - ElicitationAction::Decline => McpToolApprovalDecision::Decline { message: None }, - ElicitationAction::Cancel => McpToolApprovalDecision::Cancel, - _ => McpToolApprovalDecision::Cancel, + ElicitationAction::Decline => ReviewDecision::denied("user rejected MCP tool call"), + ElicitationAction::Cancel => ReviewDecision::Abort, + _ => ReviewDecision::Abort, } } @@ -1890,54 +1878,54 @@ fn request_user_input_response_from_elicitation_content( fn parse_mcp_tool_approval_response( response: Option, question_id: &str, -) -> McpToolApprovalDecision { +) -> ReviewDecision { let Some(response) = response else { - return McpToolApprovalDecision::Cancel; + return ReviewDecision::Abort; }; let answers = response .answers .get(question_id) .map(|answer| answer.answers.as_slice()); let Some(answers) = answers else { - return McpToolApprovalDecision::Cancel; + return ReviewDecision::Abort; }; if answers .iter() .any(|answer| answer == MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC) { - McpToolApprovalDecision::Decline { message: None } + ReviewDecision::denied("user rejected MCP tool call") } else if answers .iter() .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION) { - McpToolApprovalDecision::AcceptForSession + ReviewDecision::ApprovedForSession } else if answers .iter() .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER) { - McpToolApprovalDecision::AcceptAndRemember + ReviewDecision::ApprovedMcpPolicyAmendment } else if answers .iter() .any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT) { - McpToolApprovalDecision::Accept + ReviewDecision::Approved } else { - McpToolApprovalDecision::Cancel + ReviewDecision::Abort } } fn normalize_approval_decision_for_mode( - decision: McpToolApprovalDecision, + decision: ReviewDecision, approval_mode: AppToolApproval, -) -> McpToolApprovalDecision { +) -> ReviewDecision { if matches!( approval_mode, AppToolApproval::Prompt | AppToolApproval::Writes ) && matches!( decision, - McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember + ReviewDecision::ApprovedForSession | ReviewDecision::ApprovedMcpPolicyAmendment ) { - McpToolApprovalDecision::Accept + ReviewDecision::Approved } else { decision } @@ -1956,26 +1944,29 @@ async fn remember_mcp_tool_approval(sess: &Session, key: McpToolApprovalKey) { async fn apply_mcp_tool_approval_decision( sess: &Session, turn_context: &TurnContext, - decision: &McpToolApprovalDecision, + decision: &ReviewDecision, session_approval_key: Option, persistent_approval_key: Option, ) { match decision { - McpToolApprovalDecision::AcceptForSession => { + ReviewDecision::ApprovedForSession => { if let Some(key) = session_approval_key { remember_mcp_tool_approval(sess, key).await; } } - McpToolApprovalDecision::AcceptAndRemember => { + ReviewDecision::ApprovedMcpPolicyAmendment => { if let Some(key) = persistent_approval_key { maybe_persist_mcp_tool_approval(sess, turn_context, key).await; } else if let Some(key) = session_approval_key { remember_mcp_tool_approval(sess, key).await; } } - McpToolApprovalDecision::Accept - | McpToolApprovalDecision::Decline { .. } - | McpToolApprovalDecision::Cancel => {} + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::NetworkPolicyAmendment { .. } + | ReviewDecision::Denied { .. } + | ReviewDecision::TimedOut + | ReviewDecision::Abort => {} } } diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 7fa18ef89ca6..54f3fea0c0cf 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -320,18 +320,15 @@ fn writes_mode_does_not_require_approval_for_read_only_tools() { fn prompting_modes_do_not_allow_persistent_remember() { for approval_mode in [AppToolApproval::Prompt, AppToolApproval::Writes] { assert_eq!( - normalize_approval_decision_for_mode( - McpToolApprovalDecision::AcceptForSession, - approval_mode, - ), - McpToolApprovalDecision::Accept + normalize_approval_decision_for_mode(ReviewDecision::ApprovedForSession, approval_mode,), + ReviewDecision::Approved ); assert_eq!( normalize_approval_decision_for_mode( - McpToolApprovalDecision::AcceptAndRemember, + ReviewDecision::ApprovedMcpPolicyAmendment, approval_mode, ), - McpToolApprovalDecision::Accept + ReviewDecision::Approved ); } } @@ -1739,38 +1736,6 @@ fn guardian_mcp_review_request_ignores_untrusted_connected_account_email() { ); } -#[test] -fn guardian_review_decision_maps_to_mcp_tool_decision() { - assert_eq!( - mcp_tool_approval_decision_from_guardian(ReviewDecision::Approved), - McpToolApprovalDecision::Accept - ); - let denial = mcp_tool_approval_decision_from_guardian(ReviewDecision::denied( - "This action was rejected due to unacceptable risk.\nReason: too risky\nThe agent must not attempt to achieve the same outcome", - )); - let McpToolApprovalDecision::Decline { - message: Some(message), - } = denial - else { - panic!("guardian denial should carry a rejection message"); - }; - assert!(message.contains("Reason: too risky")); - assert!(message.contains("The agent must not attempt to achieve the same outcome")); - let timeout = mcp_tool_approval_decision_from_guardian(ReviewDecision::TimedOut); - let McpToolApprovalDecision::Decline { - message: Some(message), - } = timeout - else { - panic!("guardian timeout should carry a timeout message"); - }; - assert!(message.contains("did not finish before its deadline")); - assert!(!message.contains("unacceptable risk")); - assert_eq!( - mcp_tool_approval_decision_from_guardian(ReviewDecision::Abort), - McpToolApprovalDecision::Decline { message: None } - ); -} - #[test] fn approval_elicitation_meta_includes_connector_source_for_codex_apps() { assert_eq!( @@ -1858,7 +1823,10 @@ fn declined_elicitation_response_stays_decline() { "approval", ); - assert_eq!(response, McpToolApprovalDecision::Decline { message: None }); + assert_eq!( + response, + ReviewDecision::denied("user rejected MCP tool call") + ); } #[test] @@ -1875,7 +1843,10 @@ fn synthetic_decline_request_user_input_response_stays_decline() { "approval", ); - assert_eq!(response, McpToolApprovalDecision::Decline { message: None }); + assert_eq!( + response, + ReviewDecision::denied("user rejected MCP tool call") + ); } #[test] @@ -1891,7 +1862,7 @@ fn accepted_elicitation_response_uses_always_persist_meta() { "approval", ); - assert_eq!(response, McpToolApprovalDecision::AcceptAndRemember); + assert_eq!(response, ReviewDecision::ApprovedMcpPolicyAmendment); } #[test] @@ -1907,7 +1878,7 @@ fn accepted_elicitation_response_uses_session_persist_meta() { "approval", ); - assert_eq!(response, McpToolApprovalDecision::AcceptForSession); + assert_eq!(response, ReviewDecision::ApprovedForSession); } #[test] @@ -1921,7 +1892,7 @@ fn accepted_elicitation_without_content_defaults_to_accept() { "approval", ); - assert_eq!(response, McpToolApprovalDecision::Accept); + assert_eq!(response, ReviewDecision::Approved); } #[tokio::test] @@ -2505,7 +2476,7 @@ async fn permission_request_hook_allows_mcp_tool_call() { ) .await; - assert_eq!(decision, Some(McpToolApprovalDecision::Accept)); + assert_eq!(decision, Some(ReviewDecision::Approved)); let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log"); let inputs = log .lines() @@ -2573,7 +2544,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() { ) .await; - assert_eq!(decision, Some(McpToolApprovalDecision::Accept)); + assert_eq!(decision, Some(ReviewDecision::Approved)); let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log"); let inputs = log .lines() @@ -2656,7 +2627,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() { ) .await; - assert_eq!(decision, Some(McpToolApprovalDecision::Accept)); + assert_eq!(decision, Some(ReviewDecision::Approved)); assert!( !log_path.exists(), "remembered approval should skip PermissionRequest hooks" @@ -2741,10 +2712,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() { ) .await; - let Some(McpToolApprovalDecision::Decline { - message: Some(message), - }) = decision - else { + let Some(ReviewDecision::Denied { rejection: message }) = decision else { panic!("guardian-denied MCP approval should carry a rejection message"); }; assert!(message.contains("Reason: The tool call would expose private calendar data")); diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index d3de7ca2f877..a3a3ec65e76a 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -1013,6 +1013,7 @@ fn mcp_elicitation_response_from_guardian_decision( match decision { ReviewDecision::Approved | ReviewDecision::ApprovedForSession + | ReviewDecision::ApprovedMcpPolicyAmendment | ReviewDecision::ApprovedExecpolicyAmendment { .. } | ReviewDecision::NetworkPolicyAmendment { .. } => ElicitationResponse { action: ElicitationAction::Accept, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 9961b0b53def..5cfd3e9f49c5 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2597,7 +2597,8 @@ impl Session { strict_auto_review: false, }, }, - ReviewDecision::Abort + ReviewDecision::ApprovedMcpPolicyAmendment + | ReviewDecision::Abort | ReviewDecision::Denied { .. } | ReviewDecision::TimedOut => RequestPermissionsResponse { permissions: RequestPermissionProfile::default(), diff --git a/codex-rs/core/src/tools/approvals.rs b/codex-rs/core/src/tools/approvals.rs index 1fb02ccd887d..8d47817e3565 100644 --- a/codex-rs/core/src/tools/approvals.rs +++ b/codex-rs/core/src/tools/approvals.rs @@ -36,6 +36,7 @@ use codex_utils_path_uri::PathUri; use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; +use tracing::error; #[derive(Clone)] pub(crate) struct ApprovalContext { @@ -310,6 +311,12 @@ impl ApprovalResolution { fn into_tool_result(self) -> Result { let source = self.source; match self.decision { + ReviewDecision::ApprovedMcpPolicyAmendment => { + error!("Tool approval received ApprovedMcpPolicyAmendment"); + Err(ToolError::Rejected( + "Error while requesting approval".to_string(), + )) + } ReviewDecision::NetworkPolicyAmendment { network_policy_amendment, } if network_policy_amendment.action == NetworkPolicyRuleAction::Deny => { diff --git a/codex-rs/core/src/tools/approvals_tests.rs b/codex-rs/core/src/tools/approvals_tests.rs index af616fde6b9b..201aa029307e 100644 --- a/codex-rs/core/src/tools/approvals_tests.rs +++ b/codex-rs/core/src/tools/approvals_tests.rs @@ -20,6 +20,19 @@ fn approval_resolution_rejects_denied_network_policy_amendment() { )); } +#[test] +fn approval_resolution_rejects_mcp_policy_amendment() { + let resolution = ApprovalResolution { + decision: ReviewDecision::ApprovedMcpPolicyAmendment, + source: ApprovalResolutionSource::User, + }; + + assert!(matches!( + resolution.into_tool_result(), + Err(ToolError::Rejected(rejection)) if rejection == "Error while requesting approval" + )); +} + #[test] fn approval_resolution_aborts_turn_when_approval_is_aborted() { let resolution = ApprovalResolution { diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 6ff0b9f7739d..507d49d7faf1 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -42,6 +42,7 @@ use tokio::sync::Notify; use tokio::sync::OnceCell; use tokio::sync::RwLock; use tokio_util::sync::CancellationToken; +use tracing::error; use tracing::warn; use uuid::Uuid; @@ -985,6 +986,20 @@ impl NetworkApprovalService { PendingApprovalDecision::Deny } }, + ReviewDecision::ApprovedMcpPolicyAmendment => { + error!("Network approval received ApprovedMcpPolicyAmendment"); + if let Some(owner_call) = owner_call.as_ref() { + let rejection = "Error while requesting approval".to_string(); + let outcome = if use_guardian { + NetworkApprovalOutcome::DeniedByPolicy(rejection) + } else { + NetworkApprovalOutcome::DeniedByApproval(rejection) + }; + self.record_call_outcome(&owner_call.registration_id, outcome) + .await; + } + PendingApprovalDecision::Deny + } ReviewDecision::Denied { rejection } => { if let Some(owner_call) = owner_call.as_ref() { let outcome = if use_guardian { diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index 37b0a9c66011..21047ea52133 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -68,6 +68,7 @@ use std::sync::Arc; use std::time::Duration; use tokio::sync::RwLock; use tokio_util::sync::CancellationToken; +use tracing::error; use uuid::Uuid; pub(crate) struct PreparedUnifiedExecZshFork { @@ -537,6 +538,13 @@ impl CoreShellActionProvider { ReviewDecision::TimedOut => EscalationDecision::deny(Some( crate::guardian::guardian_timeout_message(), )), + ReviewDecision::ApprovedMcpPolicyAmendment => { + error!("Shell escalation received ApprovedMcpPolicyAmendment"); + + EscalationDecision::deny(Some( + "Error while requesting approval".to_string(), + )) + } ReviewDecision::Abort => { EscalationDecision::deny(Some("User cancelled execution".to_string())) } diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index ab2afef9df28..ddae1d7d614c 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -3861,6 +3861,10 @@ pub enum ReviewDecision { /// remainder of the session. ApprovedForSession, + /// User has approved this MCP tool call and wants to amend its policy so + /// matching future calls are automatically approved across sessions. + ApprovedMcpPolicyAmendment, + /// User chose to persist a network policy rule (allow/deny) for future /// requests to the same host. NetworkPolicyAmendment { @@ -3901,6 +3905,7 @@ impl ReviewDecision { ReviewDecision::Approved => "approved", ReviewDecision::ApprovedExecpolicyAmendment { .. } => "approved_with_amendment", ReviewDecision::ApprovedForSession => "approved_for_session", + ReviewDecision::ApprovedMcpPolicyAmendment => "approved_mcp_policy_amendment", ReviewDecision::NetworkPolicyAmendment { network_policy_amendment, } => match network_policy_amendment.action {