mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Distinguish MCP approval cancellation reasons (#54679)
## Why MCP tool calls reported every aborted approval as "user cancelled MCP tool call", including interrupted requests and automatic cancellations in `exec` mode. ## What changed Carry `user`, `interrupted`, or `non_interactive` cancellation reasons through MCP approval metadata and resolution. Mark explicit TUI cancellations as user actions, interrupted requests as interruptions, and automatic `exec` cancellations as non-interactive. Use the reason to report when a call was cancelled before approval or because interactive approval is unavailable in `exec` mode. Preserve the existing cancellation message for clients that omit the reason. ## Testing Update tests to verify that a closed approval response carries the interruption reason and that `exec` cancellation responses include the non-interactive reason. GitOrigin-RevId: be2a5f7fef539b5db2add53b46aea27e2b8a66fc
This commit is contained in:
@@ -16,6 +16,7 @@ use crate::session::session::Session;
|
||||
use crate::session::step_context::StepContext;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::tools::ApprovalContext;
|
||||
use crate::tools::ApprovalResolution;
|
||||
use crate::tools::ApprovalResolutionSource;
|
||||
use crate::tools::context::ToolCallOrigin;
|
||||
use crate::tools::hook_names::HookToolName;
|
||||
@@ -60,9 +61,11 @@ use codex_protocol::mcp::is_node_repl_backed_connector;
|
||||
use codex_protocol::mcp::is_node_repl_backed_server;
|
||||
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_KEY as MCP_TOOL_APPROVAL_KIND_KEY;
|
||||
use codex_protocol::mcp_approval_meta::APPROVAL_KIND_MCP_TOOL_CALL as MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL;
|
||||
use codex_protocol::mcp_approval_meta::CANCELLATION_REASON_KEY;
|
||||
use codex_protocol::mcp_approval_meta::CONNECTOR_DESCRIPTION_KEY as MCP_TOOL_APPROVAL_CONNECTOR_DESCRIPTION_KEY;
|
||||
use codex_protocol::mcp_approval_meta::CONNECTOR_ID_KEY as MCP_TOOL_APPROVAL_CONNECTOR_ID_KEY;
|
||||
use codex_protocol::mcp_approval_meta::CONNECTOR_NAME_KEY as MCP_TOOL_APPROVAL_CONNECTOR_NAME_KEY;
|
||||
use codex_protocol::mcp_approval_meta::McpApprovalCancellationReason;
|
||||
use codex_protocol::mcp_approval_meta::PERSIST_ALWAYS as MCP_TOOL_APPROVAL_PERSIST_ALWAYS;
|
||||
use codex_protocol::mcp_approval_meta::PERSIST_KEY as MCP_TOOL_APPROVAL_PERSIST_KEY;
|
||||
use codex_protocol::mcp_approval_meta::PERSIST_SESSION as MCP_TOOL_APPROVAL_PERSIST_SESSION;
|
||||
@@ -344,7 +347,20 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
.await
|
||||
}
|
||||
ReviewDecision::Abort => {
|
||||
let message = "user cancelled MCP tool call".to_string();
|
||||
let message = match approval.cancellation_reason {
|
||||
// Older clients do not send provenance. Preserve their
|
||||
// cancellation behavior without assigning them a user reason.
|
||||
Some(McpApprovalCancellationReason::User) | None => {
|
||||
"user cancelled MCP tool call"
|
||||
}
|
||||
Some(McpApprovalCancellationReason::Interrupted) => {
|
||||
"MCP tool call cancelled before approval"
|
||||
}
|
||||
Some(McpApprovalCancellationReason::NonInteractive) => {
|
||||
"MCP tool call cancelled: interactive approval is unavailable in exec mode"
|
||||
}
|
||||
}
|
||||
.to_string();
|
||||
notify_mcp_tool_call_skip(
|
||||
sess.as_ref(),
|
||||
turn_context.as_ref(),
|
||||
@@ -1484,6 +1500,7 @@ fn mcp_tool_approval_prompt_options(
|
||||
struct McpToolApproval {
|
||||
decision: ReviewDecision,
|
||||
source: Option<ApprovalResolutionSource>,
|
||||
cancellation_reason: Option<McpApprovalCancellationReason>,
|
||||
}
|
||||
|
||||
#[expect(clippy::too_many_arguments)]
|
||||
@@ -1544,6 +1561,7 @@ async fn maybe_request_mcp_tool_approval(
|
||||
return Some(McpToolApproval {
|
||||
decision: ReviewDecision::Approved,
|
||||
source: None,
|
||||
cancellation_reason: None,
|
||||
});
|
||||
}
|
||||
|
||||
@@ -1593,6 +1611,7 @@ async fn maybe_request_mcp_tool_approval(
|
||||
Err(ToolError::Codex(_)) => ReviewDecision::Abort,
|
||||
},
|
||||
source: approval.source,
|
||||
cancellation_reason: approval.cancellation_reason,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1601,7 +1620,7 @@ pub(crate) async fn request_mcp_tool_user_approval(
|
||||
turn_context: &TurnContext,
|
||||
call_id: &str,
|
||||
action: &ApprovalAction,
|
||||
) -> ReviewDecision {
|
||||
) -> ApprovalResolution {
|
||||
let ApprovalAction::McpToolCall {
|
||||
id,
|
||||
server,
|
||||
@@ -1624,9 +1643,13 @@ pub(crate) async fn request_mcp_tool_user_approval(
|
||||
};
|
||||
|
||||
if *approval_policy == AskForApproval::Never {
|
||||
return ReviewDecision::denied(
|
||||
"MCP tool call requires approval, but approval policy is never",
|
||||
);
|
||||
return ApprovalResolution {
|
||||
decision: ReviewDecision::denied(
|
||||
"MCP tool call requires approval, but approval policy is never",
|
||||
),
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: None,
|
||||
};
|
||||
}
|
||||
|
||||
let tool_call_mcp_elicitation_enabled = turn_context
|
||||
@@ -1660,7 +1683,7 @@ pub(crate) async fn request_mcp_tool_user_approval(
|
||||
.as_ref()
|
||||
.map(|rendered_template| rendered_template.question.as_str()),
|
||||
);
|
||||
let (request_dispatched, decision) = if tool_call_mcp_elicitation_enabled {
|
||||
let (request_dispatched, decision, cancellation_reason) = if tool_call_mcp_elicitation_enabled {
|
||||
let link_id = sess
|
||||
.mcp_tool_approval_metadata(server, id)
|
||||
.and_then(|context| context.metadata.link_id);
|
||||
@@ -1698,17 +1721,33 @@ pub(crate) async fn request_mcp_tool_user_approval(
|
||||
let outcome = sess
|
||||
.request_mcp_server_elicitation(turn_context, server.clone(), request_id, request)
|
||||
.await;
|
||||
// Missing replies are interruptions. A received Cancel needs provenance
|
||||
// from its source; RPC success alone does not establish a user action.
|
||||
let cancellation_reason = match outcome.response.as_ref() {
|
||||
None => Some(McpApprovalCancellationReason::Interrupted),
|
||||
Some(response) if response.action == ElicitationAction::Cancel => response
|
||||
.meta
|
||||
.as_ref()
|
||||
.and_then(|meta| meta.get(CANCELLATION_REASON_KEY))
|
||||
.and_then(|reason| serde_json::from_value(reason.clone()).ok()),
|
||||
Some(_) => None,
|
||||
};
|
||||
(
|
||||
outcome.sent,
|
||||
parse_mcp_tool_approval_elicitation_response(outcome.response, &question_id),
|
||||
cancellation_reason,
|
||||
)
|
||||
} else {
|
||||
if turn_context.session_source.is_non_root_agent() {
|
||||
return ReviewDecision::denied(concat!(
|
||||
"MCP tool approval via request_user_input requires the root thread. ",
|
||||
"Ask the parent agent to handle this request. ",
|
||||
"Do not retry the blocked action until the parent confirms the blocker is resolved."
|
||||
));
|
||||
return ApprovalResolution {
|
||||
decision: ReviewDecision::denied(concat!(
|
||||
"MCP tool approval via request_user_input requires the root thread. ",
|
||||
"Ask the parent agent to handle this request. ",
|
||||
"Do not retry the blocked action until the parent confirms the blocker is resolved."
|
||||
)),
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: None,
|
||||
};
|
||||
}
|
||||
let args = RequestUserInputArgs {
|
||||
questions: vec![question],
|
||||
@@ -1718,12 +1757,28 @@ pub(crate) async fn request_mcp_tool_user_approval(
|
||||
let response = sess
|
||||
.request_user_input(turn_context, call_id.to_string(), args)
|
||||
.await;
|
||||
let cancellation_reason = response
|
||||
.as_ref()
|
||||
.and_then(|accepted| accepted.response.answers.get(&question_id))
|
||||
.filter(|answer| {
|
||||
answer
|
||||
.answers
|
||||
.iter()
|
||||
.any(|answer| answer == MCP_TOOL_APPROVAL_CANCEL)
|
||||
})
|
||||
.map(|_| McpApprovalCancellationReason::User)
|
||||
.or_else(|| {
|
||||
response
|
||||
.is_none()
|
||||
.then_some(McpApprovalCancellationReason::Interrupted)
|
||||
});
|
||||
(
|
||||
true,
|
||||
parse_mcp_tool_approval_response(
|
||||
response.map(|accepted| accepted.response),
|
||||
&question_id,
|
||||
),
|
||||
cancellation_reason,
|
||||
)
|
||||
};
|
||||
let decision = normalize_approval_decision_for_mode(decision, *approval_mode);
|
||||
@@ -1735,7 +1790,15 @@ pub(crate) async fn request_mcp_tool_user_approval(
|
||||
{
|
||||
track_mcp_tool_call_elicitation(sess, turn_context, call_id, ElicitationType::Approval);
|
||||
}
|
||||
decision
|
||||
ApprovalResolution {
|
||||
cancellation_reason: if decision == ReviewDecision::Abort {
|
||||
cancellation_reason
|
||||
} else {
|
||||
None
|
||||
},
|
||||
decision,
|
||||
source: ApprovalResolutionSource::User,
|
||||
}
|
||||
}
|
||||
|
||||
fn session_mcp_tool_approval_key(
|
||||
|
||||
@@ -2330,7 +2330,11 @@ async fn dispatched_mcp_approval_with_closed_response_is_classified_as_approval(
|
||||
*session.active_turn.lock().await = None;
|
||||
assert_eq!(
|
||||
approval.await.expect("approval task should complete"),
|
||||
ReviewDecision::Abort
|
||||
ApprovalResolution {
|
||||
decision: ReviewDecision::Abort,
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: Some(McpApprovalCancellationReason::Interrupted),
|
||||
}
|
||||
);
|
||||
|
||||
item["status"] = serde_json::json!("failed");
|
||||
@@ -3002,6 +3006,7 @@ async fn permission_request_hook_allows_mcp_tool_call() {
|
||||
Some(McpToolApproval {
|
||||
decision: ReviewDecision::Approved,
|
||||
source: Some(ApprovalResolutionSource::Hook),
|
||||
cancellation_reason: None,
|
||||
})
|
||||
);
|
||||
let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log");
|
||||
@@ -3079,6 +3084,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() {
|
||||
Some(McpToolApproval {
|
||||
decision: ReviewDecision::Approved,
|
||||
source: Some(ApprovalResolutionSource::Hook),
|
||||
cancellation_reason: None,
|
||||
})
|
||||
);
|
||||
let log = std::fs::read_to_string(log_path).expect("read MCP permission hook log");
|
||||
@@ -3172,6 +3178,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() {
|
||||
Some(McpToolApproval {
|
||||
decision: ReviewDecision::Approved,
|
||||
source: None,
|
||||
cancellation_reason: None,
|
||||
})
|
||||
);
|
||||
assert!(
|
||||
@@ -3276,6 +3283,7 @@ async fn strict_auto_review_forces_guardian_for_mcp_policy_skip() {
|
||||
let Some(McpToolApproval {
|
||||
decision: ReviewDecision::Denied { rejection: message },
|
||||
source: Some(ApprovalResolutionSource::Guardian),
|
||||
cancellation_reason: None,
|
||||
}) = decision
|
||||
else {
|
||||
panic!("guardian-denied MCP approval should carry a rejection message");
|
||||
@@ -3391,6 +3399,7 @@ async fn assert_mcp_user_approval_persistence(
|
||||
Some(McpToolApproval {
|
||||
decision: expected_decision,
|
||||
source: Some(ApprovalResolutionSource::User),
|
||||
cancellation_reason: None,
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
@@ -30,6 +30,7 @@ use codex_protocol::approvals::NetworkApprovalContext;
|
||||
use codex_protocol::approvals::NetworkApprovalProtocol;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::mcp_approval_meta::McpApprovalCancellationReason;
|
||||
use codex_protocol::models::AdditionalPermissionProfile;
|
||||
use codex_protocol::openai_models::ModelInfo;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
@@ -404,12 +405,14 @@ pub(crate) enum ApprovalResolutionSource {
|
||||
pub(crate) struct ApprovalResult {
|
||||
pub(crate) result: Result<ReviewDecision, ToolError>,
|
||||
pub(crate) source: Option<ApprovalResolutionSource>,
|
||||
pub(crate) cancellation_reason: Option<McpApprovalCancellationReason>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
struct ApprovalResolution {
|
||||
decision: ReviewDecision,
|
||||
source: ApprovalResolutionSource,
|
||||
pub(crate) struct ApprovalResolution {
|
||||
pub(crate) decision: ReviewDecision,
|
||||
pub(crate) source: ApprovalResolutionSource,
|
||||
pub(crate) cancellation_reason: Option<McpApprovalCancellationReason>,
|
||||
}
|
||||
|
||||
impl ApprovalResolution {
|
||||
@@ -466,6 +469,7 @@ impl Session {
|
||||
return ApprovalResult {
|
||||
result: Err(ToolError::Rejected(reason.to_string())),
|
||||
source: None,
|
||||
cancellation_reason: None,
|
||||
};
|
||||
}
|
||||
let is_mcp_tool_call = matches!(&action, ApprovalAction::McpToolCall { .. });
|
||||
@@ -490,10 +494,12 @@ impl Session {
|
||||
Some(PermissionRequestDecision::Allow) => ApprovalResolution {
|
||||
decision: ReviewDecision::Approved,
|
||||
source: ApprovalResolutionSource::Hook,
|
||||
cancellation_reason: None,
|
||||
},
|
||||
Some(PermissionRequestDecision::Deny { message }) => ApprovalResolution {
|
||||
decision: ReviewDecision::denied(message),
|
||||
source: ApprovalResolutionSource::Hook,
|
||||
cancellation_reason: None,
|
||||
},
|
||||
None => self.request_reviewer_approval(action, &ctx).await,
|
||||
};
|
||||
@@ -502,10 +508,12 @@ impl Session {
|
||||
record_resolution(&ctx, &resolution);
|
||||
}
|
||||
let source = Some(resolution.source);
|
||||
let cancellation_reason = resolution.cancellation_reason;
|
||||
if is_mcp_tool_call && resolution.decision == ReviewDecision::ApprovedMcpPolicyAmendment {
|
||||
return ApprovalResult {
|
||||
result: Ok(resolution.decision),
|
||||
source,
|
||||
cancellation_reason,
|
||||
};
|
||||
}
|
||||
if is_network_approval {
|
||||
@@ -519,6 +527,7 @@ impl Session {
|
||||
return ApprovalResult {
|
||||
result: Ok(resolution.decision),
|
||||
source,
|
||||
cancellation_reason,
|
||||
};
|
||||
}
|
||||
(ReviewDecision::Abort, ApprovalResolutionSource::Guardian) => {
|
||||
@@ -527,6 +536,7 @@ impl Session {
|
||||
"automatic approval review was cancelled".to_string(),
|
||||
)),
|
||||
source,
|
||||
cancellation_reason,
|
||||
};
|
||||
}
|
||||
_ => {}
|
||||
@@ -535,6 +545,7 @@ impl Session {
|
||||
ApprovalResult {
|
||||
result: resolution.into_tool_result(ctx.review_context.turn().model_info()),
|
||||
source,
|
||||
cancellation_reason,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -545,14 +556,13 @@ impl Session {
|
||||
) -> ApprovalResolution {
|
||||
if let Some(decision) = self.request_guardian_approval(action.clone(), ctx).await {
|
||||
ApprovalResolution {
|
||||
cancellation_reason: (decision == ReviewDecision::Abort)
|
||||
.then_some(McpApprovalCancellationReason::Interrupted),
|
||||
decision,
|
||||
source: ApprovalResolutionSource::Guardian,
|
||||
}
|
||||
} else {
|
||||
ApprovalResolution {
|
||||
decision: self.request_user_approval(&action, ctx).await,
|
||||
source: ApprovalResolutionSource::User,
|
||||
}
|
||||
self.request_user_approval(&action, ctx).await
|
||||
}
|
||||
}
|
||||
|
||||
@@ -667,8 +677,8 @@ impl Session {
|
||||
&self,
|
||||
action: &ApprovalAction,
|
||||
ctx: &ApprovalContext,
|
||||
) -> ReviewDecision {
|
||||
match action {
|
||||
) -> ApprovalResolution {
|
||||
let decision = match action {
|
||||
ApprovalAction::ExecCommand {
|
||||
environment_id,
|
||||
command,
|
||||
@@ -760,44 +770,43 @@ impl Session {
|
||||
.clone()
|
||||
.or_else(|| ctx.approval_reason.clone());
|
||||
if *permissions_preapproved && reason.is_none() {
|
||||
return ReviewDecision::Approved;
|
||||
ReviewDecision::Approved
|
||||
} else if reason.is_some() {
|
||||
self.request_patch_approval(
|
||||
ctx.review_context.turn(),
|
||||
ctx.call_id.clone(),
|
||||
changes.as_ref().clone(),
|
||||
reason,
|
||||
/*grant_root*/ None,
|
||||
)
|
||||
.await
|
||||
} else {
|
||||
with_cached_approval(
|
||||
&self.services,
|
||||
"apply_patch",
|
||||
action.cache_keys(),
|
||||
|| async {
|
||||
self.request_patch_approval(
|
||||
ctx.review_context.turn(),
|
||||
ctx.call_id.clone(),
|
||||
changes.as_ref().clone(),
|
||||
/*reason*/ None,
|
||||
/*grant_root*/ None,
|
||||
)
|
||||
.await
|
||||
},
|
||||
)
|
||||
.await
|
||||
}
|
||||
if reason.is_some() {
|
||||
return self
|
||||
.request_patch_approval(
|
||||
ctx.review_context.turn(),
|
||||
ctx.call_id.clone(),
|
||||
changes.as_ref().clone(),
|
||||
reason,
|
||||
/*grant_root*/ None,
|
||||
)
|
||||
.await;
|
||||
}
|
||||
with_cached_approval(
|
||||
&self.services,
|
||||
"apply_patch",
|
||||
action.cache_keys(),
|
||||
|| async {
|
||||
self.request_patch_approval(
|
||||
ctx.review_context.turn(),
|
||||
ctx.call_id.clone(),
|
||||
changes.as_ref().clone(),
|
||||
/*reason*/ None,
|
||||
/*grant_root*/ None,
|
||||
)
|
||||
.await
|
||||
},
|
||||
)
|
||||
.await
|
||||
}
|
||||
ApprovalAction::McpToolCall { .. } => {
|
||||
request_mcp_tool_user_approval(
|
||||
return request_mcp_tool_user_approval(
|
||||
self,
|
||||
ctx.review_context.turn(),
|
||||
&ctx.call_id,
|
||||
action,
|
||||
)
|
||||
.await
|
||||
.await;
|
||||
}
|
||||
ApprovalAction::NetworkAccess {
|
||||
environment_id,
|
||||
@@ -826,6 +835,11 @@ impl Session {
|
||||
ApprovalAction::RequestPermissions { .. } => {
|
||||
unreachable!("permission requests are routed directly to Guardian")
|
||||
}
|
||||
};
|
||||
ApprovalResolution {
|
||||
decision,
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: None,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,6 +14,7 @@ fn approval_resolution_rejects_denied_network_policy_amendment() {
|
||||
},
|
||||
},
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: None,
|
||||
};
|
||||
|
||||
assert!(matches!(
|
||||
@@ -27,6 +28,7 @@ fn approval_resolution_rejects_mcp_policy_amendment() {
|
||||
let resolution = ApprovalResolution {
|
||||
decision: ReviewDecision::ApprovedMcpPolicyAmendment,
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: None,
|
||||
};
|
||||
|
||||
assert!(matches!(
|
||||
@@ -40,6 +42,7 @@ fn approval_resolution_aborts_turn_when_approval_is_aborted() {
|
||||
let resolution = ApprovalResolution {
|
||||
decision: ReviewDecision::Abort,
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: None,
|
||||
};
|
||||
|
||||
assert!(matches!(
|
||||
@@ -67,6 +70,7 @@ fn approval_resolution_uses_acting_model_timeout_instructions() {
|
||||
let resolution = ApprovalResolution {
|
||||
decision: ReviewDecision::TimedOut,
|
||||
source: ApprovalResolutionSource::Guardian,
|
||||
cancellation_reason: None,
|
||||
};
|
||||
|
||||
assert!(matches!(
|
||||
@@ -167,11 +171,13 @@ async fn non_utf8_cwd_preserves_approval_routing(
|
||||
ApprovalResolution {
|
||||
decision: ReviewDecision::Approved,
|
||||
source: ApprovalResolutionSource::User,
|
||||
cancellation_reason: None,
|
||||
}
|
||||
} else {
|
||||
ApprovalResolution {
|
||||
decision: ReviewDecision::Approved,
|
||||
source: ApprovalResolutionSource::Guardian,
|
||||
cancellation_reason: None,
|
||||
}
|
||||
};
|
||||
assert_eq!(approval.await, expected);
|
||||
|
||||
@@ -29,6 +29,7 @@ use std::borrow::Cow;
|
||||
|
||||
use crate::session::turn_context::TurnContext;
|
||||
pub(crate) use approvals::ApprovalContext;
|
||||
pub(crate) use approvals::ApprovalResolution;
|
||||
pub(crate) use approvals::ApprovalResolutionSource;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::exec_output::ExecToolCallOutput;
|
||||
|
||||
Reference in New Issue
Block a user