Skip to content

Support PermissionRequest updatedInput rewrites #21433

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
abhinav-oai wants to merge 1 commit into
base: abhinav/hooks-updated-input
Choose a base branch
from
Draft

Support PermissionRequest updatedInput rewrites #21433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
82 changes: 60 additions & 22 deletions codex-rs/core/src/mcp_tool_call.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ pub(crate) async fn handle_mcp_tool_call(
)
.await;

let mut invocation = invocation;
if let Some(decision) = maybe_request_mcp_tool_approval(
&sess,
turn_context,
Expand All @@ -223,9 +224,28 @@ pub(crate) async fn handle_mcp_tool_call(
&hook_tool_name,
metadata.as_ref(),
approval_mode,
/*evaluate_permission_request_hooks*/ true,
)
.await
{
let decision = match decision {
McpToolApprovalDecision::AcceptWithUpdatedInput(updated_input) => {
invocation.arguments = Some(updated_input);
maybe_request_mcp_tool_approval(
&sess,
turn_context,
&call_id,
&invocation,
&hook_tool_name,
metadata.as_ref(),
approval_mode,
/*evaluate_permission_request_hooks*/ false,
)
.await
.unwrap_or(McpToolApprovalDecision::Accept)
}
decision => decision,
};
let result = match decision {
McpToolApprovalDecision::Accept
| McpToolApprovalDecision::AcceptForSession
Expand Down Expand Up @@ -279,6 +299,9 @@ pub(crate) async fn handle_mcp_tool_call(
)
.await
}
McpToolApprovalDecision::AcceptWithUpdatedInput(_) => {
unreachable!("updated MCP tool input should be re-approved before dispatch")
}
};

let status = if result.is_ok() { "ok" } else { "error" };
Expand Down Expand Up @@ -951,6 +974,7 @@ async fn maybe_track_codex_app_used(
#[derive(Debug, Clone, PartialEq, Eq)]
enum McpToolApprovalDecision {
Accept,
AcceptWithUpdatedInput(JsonValue),
AcceptForSession,
AcceptAndRemember,
Decline { message: Option<String> },
Expand Down Expand Up @@ -1139,6 +1163,7 @@ fn mcp_tool_approval_prompt_options(
}
}

#[allow(clippy::too_many_arguments)]
async fn maybe_request_mcp_tool_approval(
sess: &Arc<Session>,
turn_context: &Arc<TurnContext>,
Expand All @@ -1147,6 +1172,7 @@ async fn maybe_request_mcp_tool_approval(
hook_tool_name: &str,
metadata: Option<&McpToolApprovalMetadata>,
approval_mode: AppToolApproval,
evaluate_permission_request_hooks: bool,
) -> Option<McpToolApprovalDecision> {
if mcp_permission_prompt_is_auto_approved(
turn_context.approval_policy.value(),
Expand Down Expand Up @@ -1199,29 +1225,40 @@ async fn maybe_request_mcp_tool_approval(
return Some(McpToolApprovalDecision::Accept);
}

match run_permission_request_hooks(
sess,
turn_context,
call_id,
PermissionRequestPayload {
tool_name: HookToolName::new(hook_tool_name),
tool_input: invocation
.arguments
.clone()
.unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())),
},
)
.await
{
Some(PermissionRequestDecision::Allow) => {
return Some(McpToolApprovalDecision::Accept);
}
Some(PermissionRequestDecision::Deny { message }) => {
return Some(McpToolApprovalDecision::Decline {
message: Some(message),
});
if evaluate_permission_request_hooks {
match run_permission_request_hooks(
sess,
turn_context,
call_id,
PermissionRequestPayload {
tool_name: HookToolName::new(hook_tool_name),
tool_input: invocation
.arguments
.clone()
.unwrap_or_else(|| serde_json::Value::Object(serde_json::Map::new())),
},
)
.await
{
Some(PermissionRequestDecision::Allow {
updated_input: Some(updated_input),
}) => {
return Some(McpToolApprovalDecision::AcceptWithUpdatedInput(
updated_input,
));
}
Some(PermissionRequestDecision::Allow {
updated_input: None,
}) => {
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
Expand Down Expand Up @@ -1967,6 +2004,7 @@ async fn apply_mcp_tool_approval_decision(
}
}
McpToolApprovalDecision::Accept
| McpToolApprovalDecision::AcceptWithUpdatedInput(_)
| McpToolApprovalDecision::Decline { .. }
| McpToolApprovalDecision::Cancel
| McpToolApprovalDecision::BlockedBySafetyMonitor(_) => {}
Expand Down
12 changes: 12 additions & 0 deletions codex-rs/core/src/mcp_tool_call_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2189,6 +2189,7 @@ async fn approve_mode_skips_when_annotations_do_not_require_approval() {
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Approve,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2262,6 +2263,7 @@ async fn guardian_mode_skips_auto_when_annotations_do_not_require_approval() {
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Auto,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2318,6 +2320,7 @@ async fn permission_request_hook_allows_mcp_tool_call() {
"mcp__memory__create_entities",
Some(&metadata),
AppToolApproval::Auto,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2378,6 +2381,7 @@ async fn permission_request_hook_uses_hook_tool_name_without_metadata() {
"mcp__memory__create_entities",
/*metadata*/ None,
AppToolApproval::Auto,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2455,6 +2459,7 @@ async fn permission_request_hook_runs_after_remembered_mcp_approval() {
"mcp__memory__create_entities",
Some(&metadata),
AppToolApproval::Auto,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2535,6 +2540,7 @@ async fn guardian_mode_mcp_denial_returns_rationale_message() {
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Auto,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2592,6 +2598,7 @@ async fn prompt_mode_waits_for_approval_when_annotations_do_not_require_approval
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Prompt,
/*evaluate_permission_request_hooks*/ true,
)
.await
})
Expand Down Expand Up @@ -2667,6 +2674,7 @@ async fn approve_mode_skips_arc_interrupt_for_model() {
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Approve,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2734,6 +2742,7 @@ async fn custom_approve_mode_skips_arc_interrupt_for_model() {
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Approve,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2801,6 +2810,7 @@ async fn approve_mode_skips_arc_interrupt_without_annotations() {
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Approve,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2878,6 +2888,7 @@ async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() {
"mcp__test__tool",
Some(&metadata),
approval_mode,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down Expand Up @@ -2981,6 +2992,7 @@ async fn approve_mode_skips_arc_and_guardian_in_every_permission_mode() {
"mcp__test__tool",
Some(&metadata),
AppToolApproval::Approve,
/*evaluate_permission_request_hooks*/ true,
)
.await;

Expand Down
2 changes: 1 addition & 1 deletion codex-rs/core/src/session/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1031,7 +1031,7 @@ async fn danger_full_access_tool_attempts_do_not_enforce_managed_network() -> an
orchestrator
.run(
&mut tool,
&(),
(),
&tool_ctx,
turn.as_ref(),
AskForApproval::Never,
Expand Down
4 changes: 2 additions & 2 deletions codex-rs/core/src/tools/handlers/apply_patch.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,7 +422,7 @@ impl ToolHandler for ApplyPatchHandler {
let out = orchestrator
.run(
&mut runtime,
&req,
req,
&tool_ctx,
turn.as_ref(),
turn.approval_policy.value(),
Expand Down Expand Up @@ -530,7 +530,7 @@ pub(crate) async fn intercept_apply_patch(
let out = orchestrator
.run(
&mut runtime,
&req,
req,
&tool_ctx,
turn.as_ref(),
turn.approval_policy.value(),
Expand Down
5 changes: 3 additions & 2 deletions codex-rs/core/src/tools/handlers/shell.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result<FunctionToolOutput, Func
} else {
effective_additional_permissions.sandbox_permissions
},
prefix_rule,
prefix_rule: prefix_rule.clone(),
})
.await;

Expand All @@ -247,6 +247,7 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result<FunctionToolOutput, Func
additional_permissions_preapproved: effective_additional_permissions
.permissions_preapproved,
justification: exec_params.justification.clone(),
prefix_rule,
exec_approval_requirement,
};
let mut orchestrator = ToolOrchestrator::new();
Expand All @@ -268,7 +269,7 @@ async fn run_exec_like(args: RunExecLikeArgs) -> Result<FunctionToolOutput, Func
let out = orchestrator
.run(
&mut runtime,
&req,
req,
&tool_ctx,
&turn,
turn.approval_policy.value(),
Expand Down
20 changes: 19 additions & 1 deletion codex-rs/core/src/tools/network_approval.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,14 +473,32 @@ impl NetworkApprovalService {
.await
{
match permission_request_decision {
PermissionRequestDecision::Allow => {
PermissionRequestDecision::Allow {
updated_input: None,
} => {
pending
.set_decision(PendingApprovalDecision::AllowOnce)
.await;
let mut pending_approvals = self.pending_host_approvals.lock().await;
pending_approvals.remove(&key);
return NetworkDecision::Allow;
}
PermissionRequestDecision::Allow {
updated_input: Some(_),
} => {
let message = "updatedInput is not supported for network approvals".to_string();
if let Some(owner_call) = owner_call.as_ref() {
self.record_call_outcome(
&owner_call.registration_id,
NetworkApprovalOutcome::DeniedByPolicy(message),
)
.await;
}
pending.set_decision(PendingApprovalDecision::Deny).await;
let mut pending_approvals = self.pending_host_approvals.lock().await;
pending_approvals.remove(&key);
return NetworkDecision::deny(REASON_NOT_ALLOWED);
}
PermissionRequestDecision::Deny { message } => {
if let Some(owner_call) = owner_call.as_ref() {
self.record_call_outcome(
Expand Down
Loading
Loading