github/codex
4
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 00:18:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

Route MCP tool calls through shared approval handling (#38108)

Browse Source 
## What changed

- Represent MCP tool calls as approval actions and route them through the
  session-level approval flow for permission hooks, reviewer selection,
  rejection handling, and resolution telemetry.
- Preserve MCP-specific user prompts, session and persistent approval choices,
  and the approval policy and reviewer captured for the tool call.

## Testing

- Add coverage for permission hooks resolving MCP calls before user or
  automatic review, persistence choices, captured policy routing, and MCP turn
  metadata with strict automatic review enabled.

GitOrigin-RevId: 8e134b3d17a19dc62b3b56076d2cd05a54c9f638
This commit is contained in:
Dylan Hurd
2026-08-12 03:00:47 +00:00
committed by copyberry
parent eb9dceba1a
commit 2230d64464
7 changed files with 618 additions and 130 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/mcp_tool_call.rs
+123 -86
View File
@@ -8,17 +8,17 @@ use crate::config::edit::ConfigEditsBuilder;
use crate::connectors;
use crate::guardian::GuardianApprovalRequest;
use crate::guardian::GuardianMcpAnnotations;
use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request;
use crate::hook_runtime::run_permission_request_hooks;
use crate::guardian::GuardianReviewContext;
use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files;
use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam;
use crate::mcp_tool_approval_templates::render_mcp_tool_approval_template;
use crate::session::session::Session;
use crate::session::step_context::StepContext;
use crate::session::turn_context::TurnContext;
use crate::tools::ApprovalContext;
use crate::tools::hook_names::HookToolName;
use crate::tools::sandboxing::PermissionRequestPayload;
use crate::tools::sandboxing::ApprovalAction;
use crate::tools::sandboxing::ToolError;
use crate::turn_metadata::McpTurnMetadataContext;
use codex_analytics::AppInvocation;
use codex_analytics::InvocationType;
@@ -31,12 +31,12 @@ use codex_connectors::AppToolPolicy;
use codex_connectors::AppToolPolicyEvaluator;
use codex_connectors::AppToolPolicyInput;
use codex_features::Feature;
use codex_hooks::PermissionRequestDecision;
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
use codex_mcp::MCP_TOOL_CODEX_APPS_META_KEY;
use codex_mcp::McpPermissionPromptAutoApproveContext;
use codex_mcp::PreparedMcpCall;
use codex_mcp::SandboxState;
use codex_mcp::ToolInfo;
use codex_mcp::auth_elicitation_completed_result;
use codex_mcp::build_auth_elicitation_plan;
use codex_mcp::mcp_permission_prompt_is_auto_approved;
@@ -72,6 +72,7 @@ use codex_protocol::request_user_input::RequestUserInputResponse;
use codex_rmcp_client::ElicitationAction;
use codex_rmcp_client::ElicitationResponse;
use codex_rollout::state_db;
use codex_tools::ToolName;
use codex_utils_absolute_path::AbsolutePathBuf;
use codex_utils_output_truncation::TruncationPolicy;
use codex_utils_output_truncation::truncate_text;
@@ -112,12 +113,14 @@ pub(crate) async fn handle_mcp_tool_call(
sess: Arc<Session>,
step_context: &Arc<StepContext>,
call_id: String,
server: String,
tool_name: String,
tool_info: &ToolInfo,
hook_tool_name: HookToolName,
invocation_tool_name: ToolName,
arguments: String,
) -> HandledMcpToolCall {
let turn_context = &step_context.turn;
let server = tool_info.server_name.clone();
let tool_name = tool_info.tool.name.to_string();
// Parse the `arguments` as JSON. An empty string is OK, but invalid JSON
// is not.
let arguments_value = if arguments.trim().is_empty() {
@@ -233,6 +236,7 @@ pub(crate) async fn handle_mcp_tool_call(
step_context,
&call_id,
&invocation,
&invocation_tool_name,
&hook_tool_name,
&metadata,
prepared_call.config(),
@@ -1282,14 +1286,13 @@ struct McpToolApprovalKey {
}
fn mcp_tool_approval_prompt_options(
session_approval_key: Option<&McpToolApprovalKey>,
persistent_approval_key: Option<&McpToolApprovalKey>,
allow_session_remember: bool,
allow_persistent_approval: bool,
tool_call_mcp_elicitation_enabled: bool,
) -> McpToolApprovalPromptOptions {
McpToolApprovalPromptOptions {
allow_session_remember: session_approval_key.is_some(),
allow_persistent_approval: tool_call_mcp_elicitation_enabled
&& persistent_approval_key.is_some(),
allow_session_remember,
allow_persistent_approval: tool_call_mcp_elicitation_enabled && allow_persistent_approval,
}
}
@@ -1299,6 +1302,7 @@ async fn maybe_request_mcp_tool_approval(
step_context: &Arc<StepContext>,
call_id: &str,
invocation: &McpInvocation,
invocation_tool_name: &ToolName,
hook_tool_name: &HookToolName,
metadata: &McpToolApprovalMetadata,
config: &codex_mcp::McpConfig,
@@ -1340,93 +1344,133 @@ async fn maybe_request_mcp_tool_approval(
return Some(ReviewDecision::Approved);
}
match run_permission_request_hooks(
sess,
turn_context,
call_id,
PermissionRequestPayload {
tool_name: hook_tool_name.clone(),
tool_input: invocation
.arguments
.clone()
.unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())),
let action = ApprovalAction::McpToolCall {
id: call_id.to_string(),
server: invocation.server.clone(),
tool_name: invocation.tool.clone(),
arguments: invocation.arguments.clone(),
connector_id: metadata.connector_id.clone(),
connector_name: metadata.connector_name.clone(),
connector_description: metadata.connector_description.clone(),
connected_account_email: (invocation.server == CODEX_APPS_MCP_SERVER_NAME)
.then(|| metadata.connected_account_email.clone())
.flatten(),
tool_title: metadata.tool_title.clone(),
tool_description: metadata.tool_description.clone(),
annotations: metadata
.annotations
.as_ref()
.map(|annotations| GuardianMcpAnnotations {
destructive_hint: annotations.destructive_hint,
open_world_hint: annotations.open_world_hint,
read_only_hint: annotations.read_only_hint,
}),
hook_tool_name: hook_tool_name.clone(),
approval_policy: config.approval_policy.value(),
reviewer: approvals_reviewer,
approval_mode: policy.mode,
allow_session_remember: session_approval_key.is_some(),
allow_persistent_approval: persistent_approval_key.is_some(),
};
let approval_context = ApprovalContext {
review_context: GuardianReviewContext::from(step_context),
call_id: call_id.to_string(),
tool_name: invocation_tool_name.clone(),
strict_auto_review: false,
approval_reason: None,
retry_reason: None,
network_approval_context: None,
};
Some(
match sess.request_approval(action, approval_context).await {
Ok(decision) => decision,
Err(ToolError::Rejected(rejection)) => ReviewDecision::denied(rejection),
Err(ToolError::Codex(_)) => ReviewDecision::Abort,
},
)
.await
{
Some(PermissionRequestDecision::Allow) => {
return Some(ReviewDecision::Approved);
}
Some(PermissionRequestDecision::Deny { message }) => {
return Some(ReviewDecision::denied(message));
}
None => {}
}
}
pub(crate) async fn request_mcp_tool_user_approval(
sess: &Session,
turn_context: &TurnContext,
call_id: &str,
action: &ApprovalAction,
) -> ReviewDecision {
let ApprovalAction::McpToolCall {
id,
server,
tool_name,
arguments,
connector_id,
connector_name,
connector_description,
connected_account_email,
tool_title,
tool_description,
approval_mode,
allow_session_remember,
allow_persistent_approval,
..
} = action
else {
unreachable!("only MCP actions can request MCP tool approval");
};
let tool_call_mcp_elicitation_enabled = turn_context
.config
.features
.enabled(Feature::ToolCallMcpElicitation);
if matches!(
config.approval_policy.value(),
AskForApproval::OnRequest | AskForApproval::Granular(_)
) && approvals_reviewer == ApprovalsReviewer::AutoReview
{
let review_id = new_guardian_review_id();
let decision = review_approval_request(
sess,
step_context,
review_id.clone(),
build_guardian_mcp_tool_review_request(call_id, invocation, Some(metadata)),
Default::default(),
)
.await;
return Some(match decision {
ReviewDecision::Abort => ReviewDecision::denied("user rejected MCP tool call"),
decision => decision,
});
}
let prompt_options = mcp_tool_approval_prompt_options(
session_approval_key.as_ref(),
persistent_approval_key.as_ref(),
*allow_session_remember,
*allow_persistent_approval,
tool_call_mcp_elicitation_enabled,
);
let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}");
let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{id}");
let rendered_template = render_mcp_tool_approval_template(
&invocation.server,
metadata.connector_id.as_deref(),
metadata.connector_name.as_deref(),
metadata.tool_title.as_deref(),
invocation.arguments.as_ref(),
server,
connector_id.as_deref(),
connector_name.as_deref(),
tool_title.as_deref(),
arguments.as_ref(),
);
let tool_params_display = rendered_template
.as_ref()
.map(|rendered_template| rendered_template.tool_params_display.clone())
.or_else(|| build_mcp_tool_approval_display_params(invocation.arguments.as_ref()));
.or_else(|| build_mcp_tool_approval_display_params(arguments.as_ref()));
let question = build_mcp_tool_approval_question(
question_id.clone(),
&invocation.server,
&invocation.tool,
metadata.connector_name.as_deref(),
server,
tool_name,
connector_name.as_deref(),
prompt_options,
rendered_template
.as_ref()
.map(|rendered_template| rendered_template.question.as_str()),
);
if tool_call_mcp_elicitation_enabled {
let request_id = rmcp::model::RequestId::String(
format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}").into(),
);
let metadata = McpToolApprovalMetadata {
annotations: None,
connector_id: connector_id.clone(),
link_id: None,
connector_name: connector_name.clone(),
connector_description: connector_description.clone(),
connected_account_email: connected_account_email.clone(),
plugin_id: None,
tool_title: tool_title.clone(),
tool_description: tool_description.clone(),
mcp_app_resource_uri: None,
codex_apps_meta: None,
openai_file_input_optional_fields: None,
};
let request_id = rmcp::model::RequestId::String(question_id.clone().into());
let request =
build_mcp_tool_approval_elicitation_request(McpToolApprovalElicitationRequest {
server: &invocation.server,
metadata: Some(metadata),
server,
metadata: Some(&metadata),
tool_params: rendered_template
.as_ref()
.and_then(|rendered_template| rendered_template.tool_params.as_ref())
.or(invocation.arguments.as_ref()),
.or(arguments.as_ref()),
tool_params_display: tool_params_display.as_deref(),
question,
message_override: rendered_template
@@ -1435,18 +1479,12 @@ async fn maybe_request_mcp_tool_approval(
prompt_options,
});
let decision = parse_mcp_tool_approval_elicitation_response(
sess.request_mcp_server_elicitation(
turn_context.as_ref(),
invocation.server.clone(),
request_id,
request,
)
.await
.response,
sess.request_mcp_server_elicitation(turn_context, server.clone(), request_id, request)
.await
.response,
&question_id,
);
let decision = normalize_approval_decision_for_mode(decision, policy.mode);
return Some(decision);
return normalize_approval_decision_for_mode(decision, *approval_mode);
}
let args = RequestUserInputArgs {
@@ -1455,13 +1493,12 @@ async fn maybe_request_mcp_tool_approval(
auto_resolution_ms: None,
};
let response = sess
.request_user_input(turn_context.as_ref(), call_id.to_string(), args)
.request_user_input(turn_context, call_id.to_string(), args)
.await;
let decision = normalize_approval_decision_for_mode(
normalize_approval_decision_for_mode(
parse_mcp_tool_approval_response(response, &question_id),
policy.mode,
);
Some(decision)
*approval_mode,
)
}
pub(crate) fn mcp_approvals_reviewer(
codex-rs/core/src/mcp_tool_call_tests.rs
+119 -11
View File
@@ -738,20 +738,13 @@ fn trusted_codex_apps_tool_question_offers_always_allow() {
#[test]
fn codex_apps_tool_question_without_elicitation_omits_always_allow() {
let session_key = McpToolApprovalKey {
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
connector_id: Some("calendar".to_string()),
tool_name: "run_action".to_string(),
};
let persistent_key = session_key.clone();
let question = build_mcp_tool_approval_question(
"q".to_string(),
CODEX_APPS_MCP_SERVER_NAME,
"run_action",
Some("Calendar"),
mcp_tool_approval_prompt_options(
Some(&session_key),
Some(&persistent_key),
/*allow_session_remember*/ true, /*allow_persistent_approval*/ true,
/*tool_call_mcp_elicitation_enabled*/ false,
),
/*question_override*/ None,
@@ -2331,6 +2324,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() {
&StepContext::for_test(Arc::clone(&turn_context)),
"call-1",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__test__tool"),
&metadata,
&approval_config(&turn_context),
@@ -2409,6 +2403,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() {
&StepContext::for_test(Arc::clone(&turn_context)),
"call-guardian",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__test__tool"),
&metadata,
&approval_config(&turn_context),
@@ -2469,6 +2464,7 @@ async fn permission_request_hook_allows_mcp_tool_call() {
&StepContext::for_test(Arc::clone(&turn_context)),
"call-mcp-hook",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__memory__create_entities"),
&metadata,
&approval_config(&turn_context),
@@ -2537,6 +2533,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() {
&StepContext::for_test(Arc::clone(&turn_context)),
"call-mcp-hook-no-metadata",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__memory__create_entities"),
&metadata,
&approval_config(&turn_context),
@@ -2620,6 +2617,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() {
&StepContext::for_test(Arc::clone(&turn_context)),
"call-mcp-remembered",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__memory__create_entities"),
&metadata,
&approval_config(&turn_context),
@@ -2635,7 +2633,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() {
}
#[tokio::test]
async fn guardian_mode_mcp_denial_returns_rationale_message() {
async fn guardian_mode_mcp_denial_uses_captured_policy_and_returns_rationale_message() {
let server = start_mock_server().await;
let guardian_request_log = mount_sse_once(
&server,
@@ -2660,7 +2658,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() {
Arc::make_mut(&mut turn_context.config)
.permissions
.approval_policy
.set(AskForApproval::OnRequest)
.set(AskForApproval::UnlessTrusted)
.expect("test setup should allow updating approval policy");
let mut config = (*turn_context.config).clone();
config.model_provider.base_url = Some(format!("{}/v1", server.uri()));
@@ -2699,15 +2697,21 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() {
codex_apps_meta: None,
openai_file_input_optional_fields: None,
};
let mut captured_mcp_config = approval_config(&turn_context);
captured_mcp_config
.approval_policy
.set(AskForApproval::OnRequest)
.expect("captured MCP policy should allow updating approval policy");
let decision = maybe_request_mcp_tool_approval(
&session,
&StepContext::for_test(Arc::clone(&turn_context)),
"call-guardian-deny",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__test__tool"),
&metadata,
&approval_config(&turn_context),
&captured_mcp_config,
McpToolApprovalPolicy::for_server(AppToolApproval::Auto),
)
.await;
@@ -2723,6 +2727,107 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() {
);
}
#[tokio::test]
async fn session_approval_preserves_mcp_session_persistence_choice() {
assert_mcp_user_approval_persistence(
MCP_TOOL_APPROVAL_PERSIST_SESSION,
ReviewDecision::ApprovedForSession,
)
.await;
}
#[tokio::test]
async fn session_approval_preserves_mcp_always_persistence_choice() {
assert_mcp_user_approval_persistence(
MCP_TOOL_APPROVAL_PERSIST_ALWAYS,
ReviewDecision::ApprovedMcpPolicyAmendment,
)
.await;
}
async fn assert_mcp_user_approval_persistence(
persistence: &'static str,
expected_decision: ReviewDecision,
) {
let (session, turn_context, rx_event) = make_session_and_context_with_rx().await;
*session.active_turn.lock().await = Some(ActiveTurn::default());
let invocation = McpInvocation {
server: "memory".to_string(),
tool: "create_entities".to_string(),
arguments: Some(serde_json::json!({ "entities": [] })),
};
let metadata = McpToolApprovalMetadata {
annotations: Some(annotations(
Some(false),
Some(true),
/*open_world*/ None,
)),
connector_id: None,
link_id: None,
connector_name: None,
connector_description: None,
connected_account_email: None,
plugin_id: None,
tool_title: Some("Create entities".to_string()),
tool_description: None,
mcp_app_resource_uri: None,
codex_apps_meta: None,
openai_file_input_optional_fields: None,
};
let approval_task = tokio::spawn({
let session = Arc::clone(&session);
let turn_context = Arc::clone(&turn_context);
async move {
maybe_request_mcp_tool_approval(
&session,
&StepContext::for_test(Arc::clone(&turn_context)),
"call-mcp-persist",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__memory__create_entities"),
&metadata,
&approval_config(&turn_context),
McpToolApprovalPolicy::for_server(AppToolApproval::Auto),
)
.await
}
});
let request = loop {
let event = tokio::time::timeout(std::time::Duration::from_secs(1), rx_event.recv())
.await
.expect("MCP approval elicitation timed out")
.expect("expected MCP approval elicitation event");
if let EventMsg::ElicitationRequest(request) = event.msg {
break request;
}
};
assert_eq!(request.server_name, "memory");
session
.resolve_elicitation(
"memory".to_string(),
rmcp::model::RequestId::String("mcp_tool_call_approval_call-mcp-persist".into()),
ElicitationResponse {
action: ElicitationAction::Accept,
content: None,
meta: Some(serde_json::json!({
MCP_TOOL_APPROVAL_PERSIST_KEY: persistence,
})),
},
)
.await
.expect("MCP approval elicitation should resolve");
assert_eq!(
tokio::time::timeout(std::time::Duration::from_secs(1), approval_task)
.await
.expect("MCP approval task timed out")
.expect("MCP approval task failed"),
Some(expected_decision),
);
}
#[tokio::test]
async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval() {
let (session, turn_context, _rx_event) = make_session_and_context_with_rx().await;
@@ -2763,6 +2868,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval
&StepContext::for_test(Arc::clone(&turn_context)),
"call-prompt",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__test__tool"),
&metadata,
&approval_config(&turn_context),
@@ -2826,6 +2932,7 @@ async fn full_access_mode_skips_mcp_tool_approval_for_all_approval_modes() {
&StepContext::for_test(Arc::clone(&turn_context)),
"call-2",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__test__tool"),
&metadata,
&approval_config(&turn_context),
@@ -2917,6 +3024,7 @@ async fn approve_mode_skips_guardian_in_every_permission_mode() {
&StepContext::for_test(Arc::clone(&turn_context)),
"call-3",
&invocation,
&ToolName::namespaced(&invocation.server, &invocation.tool),
&HookToolName::new("mcp__test__tool"),
&metadata,
&approval_config(&turn_context),
codex-rs/core/src/tools/approvals.rs
+87 -2
View File
@@ -5,8 +5,9 @@ use crate::guardian::GuardianReviewContext;
use crate::guardian::guardian_timeout_message;
use crate::guardian::new_guardian_review_id;
use crate::guardian::review_approval_request;
use crate::guardian::routes_approval_to_guardian_with_reviewer;
use crate::guardian::routes_approval_policy_to_guardian;
use crate::hook_runtime::run_permission_request_hooks;
use crate::mcp_tool_call::request_mcp_tool_user_approval;
use crate::sandboxing::SandboxPermissions;
use crate::session::session::Session;
use crate::session::turn_context::TurnContext;
@@ -19,14 +20,17 @@ use crate::tools::sandboxing::ApprovalRequestReasons;
use crate::tools::sandboxing::PermissionRequestPayload;
use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::with_cached_approval;
use codex_config::types::AppToolApproval;
use codex_hooks::PermissionRequestDecision;
use codex_otel::ToolDecisionSource;
use codex_protocol::approvals::ExecPolicyAmendment;
#[cfg(unix)]
use codex_protocol::approvals::GuardianCommandSource;
use codex_protocol::approvals::NetworkApprovalContext;
use codex_protocol::config_types::ApprovalsReviewer;
use codex_protocol::error::CodexErr;
use codex_protocol::models::AdditionalPermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::NetworkPolicyRuleAction;
use codex_protocol::protocol::ReviewDecision;
@@ -95,6 +99,25 @@ pub(crate) enum ApprovalAction {
changes: Arc<HashMap<PathBuf, FileChange>>,
permissions_preapproved: bool,
},
McpToolCall {
id: String,
server: String,
tool_name: String,
arguments: Option<serde_json::Value>,
connector_id: Option<String>,
connector_name: Option<String>,
connector_description: Option<String>,
connected_account_email: Option<String>,
tool_title: Option<String>,
tool_description: Option<String>,
annotations: Option<crate::guardian::GuardianMcpAnnotations>,
hook_tool_name: HookToolName,
approval_policy: AskForApproval,
reviewer: ApprovalsReviewer,
approval_mode: AppToolApproval,
allow_session_remember: bool,
allow_persistent_approval: bool,
},
}
#[derive(Clone, Debug, Eq, Hash, PartialEq, serde::Serialize)]
@@ -127,6 +150,16 @@ impl ApprovalAction {
tool_name: HookToolName::apply_patch(),
tool_input: serde_json::json!({ "command": patch }),
},
Self::McpToolCall {
hook_tool_name,
arguments,
..
} => PermissionRequestPayload {
tool_name: hook_tool_name.clone(),
tool_input: arguments
.clone()
.unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())),
},
}
}
@@ -164,6 +197,7 @@ impl ApprovalAction {
})],
#[cfg(unix)]
Self::Execve { .. } => Vec::new(),
Self::McpToolCall { .. } => Vec::new(),
Self::ApplyPatch {
environment_id,
files,
@@ -252,6 +286,32 @@ impl ApprovalAction {
.collect::<std::io::Result<Vec<_>>>()?,
patch,
},
Self::McpToolCall {
id,
server,
tool_name,
arguments,
connector_id,
connector_name,
connector_description,
connected_account_email,
tool_title,
tool_description,
annotations,
..
} => crate::guardian::GuardianApprovalRequest::McpToolCall {
id,
server,
tool_name,
arguments,
connector_id,
connector_name,
connector_description,
connected_account_email,
tool_title,
tool_description,
annotations,
},
})
}
}
@@ -286,7 +346,11 @@ enum ApprovalReviewer {
impl ApprovalReviewer {
fn for_turn(turn: &TurnContext) -> Self {
if routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer) {
Self::for_policy(turn.approval_policy(), turn.config.approvals_reviewer)
}
fn for_policy(approval_policy: AskForApproval, reviewer: ApprovalsReviewer) -> Self {
if routes_approval_policy_to_guardian(approval_policy, reviewer) {
Self::Guardian
} else {
Self::User
@@ -343,6 +407,7 @@ impl Session {
action: ApprovalAction,
ctx: ApprovalContext,
) -> Result<ReviewDecision, ToolError> {
let is_mcp_tool_call = matches!(&action, ApprovalAction::McpToolCall { .. });
let permission_request_run_id = match &action {
#[cfg(unix)]
ApprovalAction::Execve { approval_id, .. } => approval_id.clone(),
@@ -372,6 +437,9 @@ impl Session {
None => self.request_reviewer_approval(action, &ctx).await,
};
record_resolution(&ctx, &resolution);
if is_mcp_tool_call && resolution.decision == ReviewDecision::ApprovedMcpPolicyAmendment {
return Ok(resolution.decision);
}
resolution.into_tool_result()
}
@@ -382,6 +450,13 @@ impl Session {
) -> ApprovalResolution {
let reviewer = if ctx.strict_auto_review {
ApprovalReviewer::Guardian
} else if let ApprovalAction::McpToolCall {
approval_policy,
reviewer,
..
} = &action
{
ApprovalReviewer::for_policy(*approval_policy, *reviewer)
} else {
ApprovalReviewer::for_turn(ctx.review_context.turn())
};
@@ -465,6 +540,7 @@ impl Session {
#[cfg(unix)]
ApprovalAction::Execve { .. } => unreachable!("matched command approval"),
ApprovalAction::ApplyPatch { .. } => unreachable!("matched command approval"),
ApprovalAction::McpToolCall { .. } => unreachable!("matched command approval"),
};
let reason = ctx
.retry_reason
@@ -555,6 +631,15 @@ impl Session {
)
.await
}
ApprovalAction::McpToolCall { .. } => {
request_mcp_tool_user_approval(
self,
ctx.review_context.turn(),
&ctx.call_id,
action,
)
.await
}
}
}
}
codex-rs/core/src/tools/handlers/mcp.rs
+3 -2
View File
@@ -166,6 +166,7 @@ impl McpHandler {
session,
step_context,
call_id,
tool_name,
payload,
..
} = invocation;
@@ -185,9 +186,9 @@ impl McpHandler {
Arc::clone(&session),
&step_context,
call_id.clone(),
self.tool_info.server_name.clone(),
self.tool_info.tool.name.to_string(),
&self.tool_info,
self.hook_tool_name(),
tool_name,
payload,
)
.await;
codex-rs/core/src/tools/mod.rs
+1
View File
@@ -21,6 +21,7 @@ mod tool_namespaces_info;
use std::borrow::Cow;
use crate::session::turn_context::TurnContext;
pub(crate) use approvals::ApprovalContext;
use codex_features::Feature;
use codex_protocol::exec_output::ExecToolCallOutput;
use codex_protocol::openai_models::ToolMode;
codex-rs/core/tests/suite/hooks_mcp.rs
+198 -3
View File
@@ -6,10 +6,13 @@ use std::time::Duration;
use anyhow::Context;
use anyhow::Result;
use codex_config::types::AppToolApproval;
use codex_config::types::ApprovalsReviewer;
use codex_config::types::McpServerConfig;
use codex_config::types::McpServerTransportConfig;
use codex_core::config::Config;
use codex_features::Feature;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use core_test_support::hooks::trust_discovered_hooks;
use core_test_support::responses::ev_assistant_message;
use core_test_support::responses::ev_completed;
@@ -20,6 +23,7 @@ use core_test_support::responses::mount_sse_sequence;
use core_test_support::responses::sse;
use core_test_support::responses::start_mock_server;
use core_test_support::skip_if_no_network;
use core_test_support::skip_if_wine_exec;
use core_test_support::stdio_server_bin;
use core_test_support::test_codex::test_codex;
use core_test_support::wait_for_mcp_server;
@@ -27,6 +31,9 @@ use pretty_assertions::assert_eq;
use serde_json::Value;
use serde_json::json;
use super::rmcp_client::remote_aware_environment_id;
use super::rmcp_client::remote_aware_stdio_server_bin;
const RMCP_SERVER: &str = "rmcp";
const RMCP_PREFIXED_NAMESPACE: &str = "mcp__rmcp";
const RMCP_UNPREFIXED_NAMESPACE: &str = "rmcp";
@@ -34,6 +41,12 @@ const RMCP_ECHO_TOOL_NAME: &str = "mcp__rmcp__echo";
const RMCP_HOOK_MATCHER: &str = RMCP_ECHO_TOOL_NAME;
const RMCP_ECHO_MESSAGE: &str = "hook e2e ping";
#[derive(Clone, Copy)]
enum PermissionRequestHookOutcome {
Allow,
Deny(&'static str),
}
fn enable_mcp_tool_name_features(config: &mut Config, prefix_mcp_tool_names: bool) {
if !prefix_mcp_tool_names {
let _ = config.features.enable(Feature::NonPrefixedMcpToolNames);
@@ -170,6 +183,56 @@ print(json.dumps({{
Ok(())
}
fn write_permission_request_hook(home: &Path, outcome: PermissionRequestHookOutcome) -> Result<()> {
let script_path = home.join("permission_request_hook.py");
let log_path = home.join("permission_request_hook_log.jsonl");
let decision = match outcome {
PermissionRequestHookOutcome::Allow => json!({ "behavior": "allow" }),
PermissionRequestHookOutcome::Deny(message) => {
json!({ "behavior": "deny", "message": message })
}
};
let hook_output = json!({
"hookSpecificOutput": {
"hookEventName": "PermissionRequest",
"decision": decision,
}
});
let python_output_literal = serde_json::to_string(&hook_output.to_string())
.context("serialize MCP permission request hook output")?;
let script = format!(
r#"import json
from pathlib import Path
import sys
payload = json.load(sys.stdin)
with Path(r"{log_path}").open("a", encoding="utf-8") as handle:
handle.write(json.dumps(payload) + "\n")
print({python_output_literal})
"#,
log_path = log_path.display(),
);
let hooks = json!({
"hooks": {
"PermissionRequest": [{
"matcher": RMCP_HOOK_MATCHER,
"hooks": [{
"type": "command",
"command": format!("python3 {}", script_path.display()),
"statusMessage": "running MCP permission request hook",
}]
}]
}
});
fs::write(&script_path, script).context("write MCP permission request hook script")?;
fs::write(home.join("hooks.json"), hooks.to_string())
.context("write MCP permission request hooks")?;
Ok(())
}
fn read_hook_inputs(home: &Path, log_name: &str) -> Result<Vec<Value>> {
fs::read_to_string(home.join(log_name))
.with_context(|| format!("read {log_name}"))?
@@ -179,7 +242,12 @@ fn read_hook_inputs(home: &Path, log_name: &str) -> Result<Vec<Value>> {
.collect()
}
fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode: AppToolApproval) {
fn insert_rmcp_test_server(
config: &mut Config,
command: String,
approval_mode: AppToolApproval,
environment_id: String,
) {
let mut servers = config.mcp_servers.get().clone();
servers.insert(
RMCP_SERVER.to_string(),
@@ -192,7 +260,7 @@ fn insert_rmcp_test_server(config: &mut Config, command: String, approval_mode:
env_vars: Vec::new(),
cwd: None,
},
environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
environment_id,
enabled: true,
required: false,
supports_parallel_tool_calls: false,
@@ -223,7 +291,134 @@ fn enable_hooks_and_rmcp_server(
) {
trust_discovered_hooks(config);
enable_mcp_tool_name_features(config, prefix_mcp_tool_names);
insert_rmcp_test_server(config, rmcp_test_server_bin, approval_mode);
insert_rmcp_test_server(
config,
rmcp_test_server_bin,
approval_mode,
codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
);
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn permission_request_hook_allows_mcp_tool_without_user_or_guardian_review() -> Result<()> {
run_mcp_permission_request_hook_test(PermissionRequestHookOutcome::Allow).await
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn permission_request_hook_denies_mcp_tool_without_user_or_guardian_review() -> Result<()> {
run_mcp_permission_request_hook_test(PermissionRequestHookOutcome::Deny(
"MCP tool access denied by the integration-test hook",
))
.await
}
async fn run_mcp_permission_request_hook_test(outcome: PermissionRequestHookOutcome) -> Result<()> {
skip_if_wine_exec!(
Ok(()),
"requires a Windows test_stdio_server in the Wine-exec environment"
);
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
let call_id = match outcome {
PermissionRequestHookOutcome::Allow => "permissionrequest-rmcp-allow",
PermissionRequestHookOutcome::Deny(_) => "permissionrequest-rmcp-deny",
};
let arguments = json!({ "message": RMCP_ECHO_MESSAGE }).to_string();
let rmcp_test_server_bin = remote_aware_stdio_server_bin()?;
let mut builder = test_codex()
.with_pre_build_hook(move |home| {
write_permission_request_hook(home, outcome)
.expect("failed to write MCP permission request hook fixture");
})
.with_config(move |config| {
trust_discovered_hooks(config);
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
insert_rmcp_test_server(
config,
rmcp_test_server_bin,
AppToolApproval::Prompt,
remote_aware_environment_id(),
);
});
let test = builder.build_with_remote_and_local_env(&server).await?;
wait_for_mcp_server(&test.codex, RMCP_SERVER).await?;
let responses = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-mcp-permission-hook-1"),
ev_function_call_with_namespace(
call_id,
RMCP_PREFIXED_NAMESPACE,
"echo",
&arguments,
),
ev_completed("resp-mcp-permission-hook-1"),
]),
sse(vec![
ev_response_created("resp-mcp-permission-hook-2"),
ev_assistant_message("msg-mcp-permission-hook", "done"),
ev_completed("resp-mcp-permission-hook-2"),
]),
],
)
.await;
test.submit_turn_with_approval_and_permission_profile(
"call the rmcp echo tool with the MCP permission request hook",
AskForApproval::OnRequest,
PermissionProfile::Disabled,
)
.await?;
let requests = responses.requests();
assert_eq!(requests.len(), 2);
assert!(
requests.iter().all(|request| {
request.body_json()["client_metadata"]["x-openai-subagent"].as_str() != Some("guardian")
}),
"a permission request hook should resolve MCP approval before Guardian review",
);
let output_item = requests[1].function_call_output(call_id);
let output = output_item
.get("output")
.and_then(Value::as_str)
.expect("MCP tool output should be a string");
match outcome {
PermissionRequestHookOutcome::Allow => assert!(
output.contains(&format!("ECHOING: {RMCP_ECHO_MESSAGE}")),
"an allowed MCP tool should execute",
),
PermissionRequestHookOutcome::Deny(message) => assert!(
output.contains(message),
"a denied MCP tool should surface the hook's rejection message",
),
}
let hook_inputs =
read_hook_inputs(test.codex_home_path(), "permission_request_hook_log.jsonl")?;
assert_eq!(hook_inputs.len(), 1);
assert_eq!(
json!({
"hook_event_name": hook_inputs[0]["hook_event_name"],
"tool_name": hook_inputs[0]["tool_name"],
"tool_input": hook_inputs[0]["tool_input"],
}),
json!({
"hook_event_name": "PermissionRequest",
"tool_name": RMCP_ECHO_TOOL_NAME,
"tool_input": { "message": RMCP_ECHO_MESSAGE },
}),
);
assert!(
hook_inputs[0].get("tool_use_id").is_none(),
"PermissionRequest input should not include a tool_use_id",
);
Ok(())
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
codex-rs/core/tests/suite/mcp_turn_metadata.rs
+87 -26
View File
@@ -14,11 +14,15 @@ use codex_protocol::config_types::ModeKind;
use codex_protocol::config_types::Settings;
use codex_protocol::items::McpToolCallStatus;
use codex_protocol::items::TurnItem;
use codex_protocol::models::NetworkPermissions;
use codex_protocol::models::PermissionProfile;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::ElicitationAction;
use codex_protocol::protocol::EventMsg;
use codex_protocol::protocol::Op;
use codex_protocol::request_permissions::PermissionGrantScope;
use codex_protocol::request_permissions::RequestPermissionProfile;
use codex_protocol::request_permissions::RequestPermissionsResponse;
use codex_protocol::request_user_input::RequestUserInputAnswer;
use codex_protocol::request_user_input::RequestUserInputResponse;
use codex_protocol::user_input::UserInput;
@@ -156,7 +160,11 @@ async fn wait_for_mcp_tool_call_item(
}
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> Result<()> {
#[test_case(false; "without strict auto review")]
#[test_case(true; "with strict auto review")]
async fn approved_mcp_tool_call_metadata_records_prior_user_input_request(
strict_auto_review: bool,
) -> Result<()> {
skip_if_no_network!(Ok(()));
let server = start_mock_server().await;
@@ -166,36 +174,65 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R
"title": "Lunch",
"starts_at": "2026-03-10T12:00:00Z"
}))?;
let mock = mount_sse_sequence(
&server,
vec![
sse(vec![
ev_response_created("resp-1"),
ev_function_call_with_namespace(
call_id,
SEARCH_CALENDAR_NAMESPACE,
SEARCH_CALENDAR_CREATE_TOOL,
&calendar_args,
),
ev_completed("resp-1"),
]),
sse(vec![
ev_response_created("resp-2"),
ev_assistant_message("msg-1", "done"),
ev_completed("resp-2"),
]),
],
)
.await;
let mut response_sequence = Vec::new();
if strict_auto_review {
let requested_permissions = RequestPermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
..Default::default()
};
let request_permissions_args = json!({
"reason": "Enable strict auto review before the MCP approval",
"permissions": requested_permissions,
});
response_sequence.push(sse(vec![
ev_response_created("resp-permissions"),
ev_function_call(
"calendar-strict-permissions",
"request_permissions",
&serde_json::to_string(&request_permissions_args)?,
),
ev_completed("resp-permissions"),
]));
}
response_sequence.extend([
sse(vec![
ev_response_created("resp-1"),
ev_function_call_with_namespace(
call_id,
SEARCH_CALENDAR_NAMESPACE,
SEARCH_CALENDAR_CREATE_TOOL,
&calendar_args,
),
ev_completed("resp-1"),
]),
sse(vec![
ev_response_created("resp-2"),
ev_assistant_message("msg-1", "done"),
ev_completed("resp-2"),
]),
]);
let mock = mount_sse_sequence(&server, response_sequence).await;
let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone())
.with_config(|config| {
// Use the opposite global reviewer so this route must come from apps._default.
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
.with_config(move |config| {
// The permission grant needs user review before strict review applies to the turn.
config.approvals_reviewer = if strict_auto_review {
ApprovalsReviewer::User
} else {
ApprovalsReviewer::AutoReview
};
config
.features
.enable(Feature::ToolCallMcpElicitation)
.expect("test config should allow feature update");
if strict_auto_review {
config
.features
.enable(Feature::RequestPermissionsTool)
.expect("test config should allow feature update");
}
set_default_app_approval_mode_and_reviewer(
config,
AppToolApproval::Prompt,
@@ -213,6 +250,30 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R
)
.await?;
if strict_auto_review {
let event = wait_for_event(&test.codex, |event| {
matches!(
event,
EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_)
)
})
.await;
let EventMsg::RequestPermissions(request) = event else {
panic!("expected permission request before MCP approval, received {event:?}");
};
assert_eq!(request.call_id, "calendar-strict-permissions");
test.codex
.submit(Op::RequestPermissionsResponse {
id: request.call_id,
response: RequestPermissionsResponse {
permissions: request.permissions,
scope: PermissionGrantScope::Turn,
strict_auto_review: true,
},
})
.await?;
}
let EventMsg::McpToolCallBegin(begin) = wait_for_event(&test.codex, |event| {
matches!(event, EventMsg::McpToolCallBegin(_))
})
@@ -248,7 +309,7 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R
})
.await;
assert_eq!(mock.requests().len(), 2);
assert_eq!(mock.requests().len(), 2 + usize::from(strict_auto_review));
let apps_tool_call = recorded_apps_tool_call_by_call_id(&server, call_id).await;
assert_eq!(