Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 19 additions & 14 deletions codex-rs/core/src/hook_runtime.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use codex_hooks::PermissionRequestRequest;
use codex_hooks::PostToolUseOutcome;
use codex_hooks::PostToolUseRequest;
use codex_hooks::PreToolUseOutcome;
use codex_hooks::PreToolUsePermissionDecision;
use codex_hooks::PreToolUseRequest;
use codex_hooks::SessionStartOutcome;
use codex_hooks::UserPromptSubmitOutcome;
Expand Down Expand Up @@ -140,7 +141,7 @@ pub(crate) async fn run_pre_tool_use_hooks(
tool_use_id: String,
tool_name: &HookToolName,
tool_input: &Value,
) -> Option<String> {
) -> Result<Option<PreToolUsePermissionDecision>, String> {
let request = PreToolUseRequest {
session_id: sess.conversation_id,
turn_id: turn_context.sub_id.clone(),
Expand All @@ -161,24 +162,28 @@ pub(crate) async fn run_pre_tool_use_hooks(
hook_events,
should_block,
block_reason,
permission_decision,
} = hooks.run_pre_tool_use(request).await;
emit_hook_completed_events(sess, turn_context, hook_events).await;

if should_block {
block_reason.map(|reason| {
if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch")
&& let Some(command) = tool_input.get("command").and_then(Value::as_str)
{
format!("Command blocked by PreToolUse hook: {reason}. Command: {command}")
} else {
format!(
"Tool call blocked by PreToolUse hook: {reason}. Tool: {}",
tool_name.name()
)
}
})
Err(block_reason.map_or_else(
|| "Tool call blocked by PreToolUse hook".to_string(),
|reason| {
if (tool_name.name() == "Bash" || tool_name.name() == "apply_patch")
&& let Some(command) = tool_input.get("command").and_then(Value::as_str)
{
format!("Command blocked by PreToolUse hook: {reason}. Command: {command}")
} else {
format!(
"Tool call blocked by PreToolUse hook: {reason}. Tool: {}",
tool_name.name()
)
}
},
))
} else {
None
Ok(permission_decision)
}
}

Expand Down
92 changes: 69 additions & 23 deletions codex-rs/core/src/mcp_tool_call.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ use codex_analytics::build_track_events_context;
use codex_config::types::AppToolApproval;
use codex_features::Feature;
use codex_hooks::PermissionRequestDecision;
use codex_hooks::PreToolUsePermissionDecision;
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
use codex_mcp::McpPermissionPromptAutoApproveContext;
use codex_mcp::SandboxState;
Expand Down Expand Up @@ -87,6 +88,7 @@ const MCP_TOOL_CALL_EVENT_RESULT_MAX_BYTES: usize = DEFAULT_OUTPUT_BYTES_CAP;

/// Handles the specified tool call dispatches the appropriate
/// `McpToolCallBegin` and `McpToolCallEnd` events to the `Session`.
#[allow(clippy::too_many_arguments)]
pub(crate) async fn handle_mcp_tool_call(
sess: Arc<Session>,
turn_context: &Arc<TurnContext>,
Expand All @@ -95,6 +97,7 @@ pub(crate) async fn handle_mcp_tool_call(
tool_name: String,
hook_tool_name: String,
arguments: String,
pre_tool_use_permission_decision: Option<PreToolUsePermissionDecision>,
) -> HandledMcpToolCall {
// Parse the `arguments` as JSON. An empty string is OK, but invalid JSON
// is not.
Expand Down Expand Up @@ -191,14 +194,15 @@ pub(crate) async fn handle_mcp_tool_call(
});
notify_mcp_tool_call_event(sess.as_ref(), turn_context.as_ref(), tool_call_begin_event).await;

if let Some(decision) = maybe_request_mcp_tool_approval(
if let Some(decision) = maybe_request_mcp_tool_approval_with_pre_tool_use_decision(
&sess,
turn_context,
&call_id,
&invocation,
&hook_tool_name,
metadata.as_ref(),
approval_mode,
pre_tool_use_permission_decision.as_ref(),
)
.await
{
Expand Down Expand Up @@ -1051,6 +1055,7 @@ fn mcp_tool_approval_prompt_options(
}
}

#[cfg(test)]
async fn maybe_request_mcp_tool_approval(
sess: &Arc<Session>,
turn_context: &Arc<TurnContext>,
Expand All @@ -1060,27 +1065,65 @@ async fn maybe_request_mcp_tool_approval(
metadata: Option<&McpToolApprovalMetadata>,
approval_mode: AppToolApproval,
) -> Option<McpToolApprovalDecision> {
maybe_request_mcp_tool_approval_with_pre_tool_use_decision(
sess,
turn_context,
call_id,
invocation,
hook_tool_name,
metadata,
approval_mode,
None,
)
.await
}

#[allow(clippy::too_many_arguments)]
async fn maybe_request_mcp_tool_approval_with_pre_tool_use_decision(
sess: &Arc<Session>,
turn_context: &Arc<TurnContext>,
call_id: &str,
invocation: &McpInvocation,
hook_tool_name: &str,
metadata: Option<&McpToolApprovalMetadata>,
approval_mode: AppToolApproval,
pre_tool_use_permission_decision: Option<&PreToolUsePermissionDecision>,
) -> Option<McpToolApprovalDecision> {
if matches!(
pre_tool_use_permission_decision,
Some(PreToolUsePermissionDecision::Allow { .. })
) {
return None;
}

let force_user_prompt = matches!(
pre_tool_use_permission_decision,
Some(PreToolUsePermissionDecision::Ask { .. })
);
if mcp_permission_prompt_is_auto_approved(
turn_context.approval_policy.value(),
&turn_context.permission_profile(),
McpPermissionPromptAutoApproveContext {
approvals_reviewer: Some(turn_context.config.approvals_reviewer),
tool_approval_mode: Some(approval_mode),
},
) {
) && !force_user_prompt
{
return None;
}

let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref());
let approval_required = requires_mcp_tool_approval(annotations);
if !approval_required && approval_mode != AppToolApproval::Prompt {
if !approval_required && approval_mode != AppToolApproval::Prompt && !force_user_prompt {
return None;
}

let mut monitor_reason = None;
let mut monitor_reason = pre_tool_use_permission_decision
.and_then(PreToolUsePermissionDecision::reason)
.map(ToString::to_string);
let auto_approved_by_policy = approval_mode == AppToolApproval::Approve;

if auto_approved_by_policy {
if auto_approved_by_policy && !force_user_prompt {
match maybe_monitor_auto_approved_mcp_tool_call(
sess,
turn_context,
Expand All @@ -1105,7 +1148,8 @@ async fn maybe_request_mcp_tool_approval(
let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode);
let persistent_approval_key =
persistent_mcp_tool_approval_key(invocation, metadata, approval_mode);
if let Some(key) = session_approval_key.as_ref()
if !force_user_prompt
&& let Some(key) = session_approval_key.as_ref()
&& mcp_tool_approval_is_remembered(sess, key).await
{
return Some(McpToolApprovalDecision::Accept);
Expand All @@ -1114,31 +1158,33 @@ async fn maybe_request_mcp_tool_approval(
let approval_request =
build_mcp_tool_approval_request(call_id, hook_tool_name, invocation, metadata);

match run_permission_request_hooks(sess, turn_context, call_id, {
let Some(payload) = approval_request.permission_request_payload() else {
unreachable!("MCP approvals always project a permission request payload");
};
payload
})
.await
{
Some(PermissionRequestDecision::Allow) => {
return Some(McpToolApprovalDecision::Accept);
}
Some(PermissionRequestDecision::Deny { message }) => {
return Some(McpToolApprovalDecision::Decline {
message: Some(message),
});
if !force_user_prompt {
match run_permission_request_hooks(sess, turn_context, call_id, {
let Some(payload) = approval_request.permission_request_payload() else {
unreachable!("MCP approvals always project a permission request payload");
};
payload
})
.await
{
Some(PermissionRequestDecision::Allow) => {
return Some(McpToolApprovalDecision::Accept);
}
Some(PermissionRequestDecision::Deny { message }) => {
return Some(McpToolApprovalDecision::Decline {
message: Some(message),
});
}
None => {}
}
None => {}
}

let tool_call_mcp_elicitation_enabled = turn_context
.config
.features
.enabled(Feature::ToolCallMcpElicitation);

if routes_approval_to_guardian(turn_context) {
if !force_user_prompt && routes_approval_to_guardian(turn_context) {
let review_id = new_guardian_review_id();
let decision = review_approval_request(
sess,
Expand Down
41 changes: 41 additions & 0 deletions codex-rs/core/src/mcp_tool_call_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2090,6 +2090,47 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() {
assert_eq!(decision, None);
}

#[tokio::test]
async fn pre_tool_use_allow_skips_mcp_approval_flow() {
let (session, turn_context) = make_session_and_context().await;
let session = Arc::new(session);
let turn_context = Arc::new(turn_context);
let invocation = McpInvocation {
server: "custom_server".to_string(),
tool: "dangerous_tool".to_string(),
arguments: None,
};
let metadata = McpToolApprovalMetadata {
annotations: Some(annotations(
Some(false),
Some(true),
/*open_world*/ None,
)),
connector_id: None,
connector_name: None,
connector_description: None,
tool_title: Some("Dangerous Tool".to_string()),
tool_description: None,
mcp_app_resource_uri: None,
codex_apps_meta: None,
openai_file_input_params: None,
};

let decision = maybe_request_mcp_tool_approval_with_pre_tool_use_decision(
&session,
&turn_context,
"call-pretool-allow",
&invocation,
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Prompt,
Some(&codex_hooks::PreToolUsePermissionDecision::Allow { reason: None }),
)
.await;

assert_eq!(decision, None);
}

#[tokio::test]
async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() {
use wiremock::Mock;
Expand Down
Loading
Loading