Use ReviewDecision for MCP tool approvals (#38081)

## What changed

- Add `ApprovedMcpPolicyAmendment` to represent MCP approvals that persist across sessions.
- Route MCP approval responses directly through the shared `ReviewDecision` type while preserving session-only approvals, rejection reasons, timeouts, and cancellations.
- Reject the MCP-only policy decision if it reaches command, shell, network, or other tool approval paths.

## Testing

- Update MCP approval parsing, normalization, hook, and Guardian tests for the shared decisions.
- Verify generic tool approvals reject MCP policy amendments.

GitOrigin-RevId: b8cc931dc61d4c794915145273ede82b9eb30fc8
This commit is contained in:
Dylan Hurd
2026-08-11 22:25:14 +00:00
committed by copyberry
parent 7c47952f7c
commit 67afc79674
17 changed files with 154 additions and 120 deletions
@@ -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.",
@@ -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.",
@@ -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.",
@@ -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";
@@ -82,6 +82,9 @@ impl From<CoreReviewDecision> 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 {
+1
View File
@@ -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 => {
+58 -67
View File
@@ -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
}
McpToolApprovalDecision::Cancel => {
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
}
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<String> },
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<McpToolApprovalDecision> {
) -> Option<ReviewDecision> {
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<ElicitationResponse>,
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<RequestUserInputResponse>,
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<McpToolApprovalKey>,
persistent_approval_key: Option<McpToolApprovalKey>,
) {
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 => {}
}
}
+19 -51
View File
@@ -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"));
+1
View File
@@ -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,
+2 -1
View File
@@ -2597,7 +2597,8 @@ impl Session {
strict_auto_review: false,
},
},
ReviewDecision::Abort
ReviewDecision::ApprovedMcpPolicyAmendment
| ReviewDecision::Abort
| ReviewDecision::Denied { .. }
| ReviewDecision::TimedOut => RequestPermissionsResponse {
permissions: RequestPermissionProfile::default(),
+7
View File
@@ -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<ReviewDecision, ToolError> {
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 => {
@@ -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 {
@@ -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 {
@@ -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()))
}
+5
View File
@@ -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 {