diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index d142d33a2ffc..1dd8cf718060 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -12,7 +12,6 @@ use codex_protocol::protocol::ExecApprovalRequestEvent; use codex_protocol::protocol::McpInvocation; use codex_protocol::protocol::Op; use codex_protocol::protocol::RequestUserInputEvent; -use codex_protocol::protocol::ReviewDecision; use codex_protocol::protocol::SessionSource; use codex_protocol::protocol::SubAgentSource; use codex_protocol::protocol::Submission; @@ -23,6 +22,7 @@ use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputArgs; use codex_protocol::request_user_input::RequestUserInputResponse; use codex_protocol::user_input::UserInput; +use codex_shell_command::parse_command::shlex_join; use serde_json::Value; use std::time::Duration; use tokio::sync::Mutex; @@ -35,12 +35,10 @@ use crate::guardian::GuardianApprovalRequest; use crate::guardian::new_guardian_review_id; use crate::guardian::routes_approval_to_guardian; use crate::guardian::spawn_approval_request_review; -use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT; -use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION; -use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; -use crate::mcp_tool_call::build_guardian_mcp_tool_review_request; +use crate::mcp_tool_call::build_mcp_tool_approval_request; use crate::mcp_tool_call::is_mcp_tool_approval_question_id; use crate::mcp_tool_call::lookup_mcp_tool_metadata; +use crate::mcp_tool_call::mcp_tool_approval_compat_response; use crate::session::Codex; use crate::session::CodexSpawnArgs; use crate::session::CodexSpawnOk; @@ -452,25 +450,28 @@ async fn handle_exec_approval( available_decisions, .. } = event; + let hook_command = shlex_join(&command); + let approval_request = GuardianApprovalRequest::Shell { + id: call_id.clone(), + command, + hook_command, + cwd, + sandbox_permissions: if additional_permissions.is_some() { + crate::sandboxing::SandboxPermissions::WithAdditionalPermissions + } else { + crate::sandboxing::SandboxPermissions::UseDefault + }, + additional_permissions, + justification: None, + }; let decision = if routes_approval_to_guardian(parent_ctx) { let review_cancel = cancel_token.child_token(); let review_rx = spawn_approval_request_review( Arc::clone(parent_session), Arc::clone(parent_ctx), new_guardian_review_id(), - GuardianApprovalRequest::Shell { - id: call_id.clone(), - command, - cwd, - sandbox_permissions: if additional_permissions.is_some() { - crate::sandboxing::SandboxPermissions::WithAdditionalPermissions - } else { - crate::sandboxing::SandboxPermissions::UseDefault - }, - additional_permissions, - justification: None, - }, - reason, + approval_request.clone(), + reason.clone(), GuardianApprovalRequestSource::DelegatedSubagent, review_cancel.clone(), ); @@ -484,17 +485,15 @@ async fn handle_exec_approval( .await } else { await_approval_with_cancel( - parent_session.request_command_approval( + parent_session.request_command_approval_for_request( parent_ctx, - call_id, + approval_request, approval_id, - command, - cwd, reason, network_approval_context, proposed_execpolicy_amendment, - additional_permissions, available_decisions, + /*fallback_cwd*/ None, ), parent_session, &approval_id_for_op, @@ -530,49 +529,50 @@ async fn handle_patch_approval( .. } = event; let approval_id = call_id.clone(); - let guardian_decision = if routes_approval_to_guardian(parent_ctx) { - let files = changes + let patch = changes + .iter() + .map(|(path, change)| match change { + codex_protocol::protocol::FileChange::Add { content } => { + format!("*** Add File: {}\n{}", path.display(), content) + } + codex_protocol::protocol::FileChange::Delete { content } => { + format!("*** Delete File: {}\n{}", path.display(), content) + } + codex_protocol::protocol::FileChange::Update { + unified_diff, + move_path, + } => { + if let Some(move_path) = move_path { + format!( + "*** Update File: {}\n*** Move to: {}\n{}", + path.display(), + move_path.display(), + unified_diff + ) + } else { + format!("*** Update File: {}\n{}", path.display(), unified_diff) + } + } + }) + .collect::>() + .join("\n"); + let approval_request = GuardianApprovalRequest::ApplyPatch { + id: approval_id.clone(), + cwd: parent_ctx.cwd.clone(), + files: changes .keys() .map(|path| parent_ctx.cwd.join(path)) - .collect::>(); + .collect::>(), + changes: changes.clone(), + patch, + }; + let guardian_decision = if routes_approval_to_guardian(parent_ctx) { let review_cancel = cancel_token.child_token(); - let patch = changes - .iter() - .map(|(path, change)| match change { - codex_protocol::protocol::FileChange::Add { content } => { - format!("*** Add File: {}\n{}", path.display(), content) - } - codex_protocol::protocol::FileChange::Delete { content } => { - format!("*** Delete File: {}\n{}", path.display(), content) - } - codex_protocol::protocol::FileChange::Update { - unified_diff, - move_path, - } => { - if let Some(move_path) = move_path { - format!( - "*** Update File: {}\n*** Move to: {}\n{}", - path.display(), - move_path.display(), - unified_diff - ) - } else { - format!("*** Update File: {}\n{}", path.display(), unified_diff) - } - } - }) - .collect::>() - .join("\n"); let review_rx = spawn_approval_request_review( Arc::clone(parent_session), Arc::clone(parent_ctx), new_guardian_review_id(), - GuardianApprovalRequest::ApplyPatch { - id: approval_id.clone(), - cwd: parent_ctx.cwd.clone(), - files, - patch, - }, + approval_request.clone(), reason.clone(), GuardianApprovalRequestSource::DelegatedSubagent, review_cancel.clone(), @@ -594,7 +594,7 @@ async fn handle_patch_approval( decision } else { let decision_rx = parent_session - .request_patch_approval(parent_ctx, call_id, changes, reason, grant_root) + .request_patch_approval_for_request(parent_ctx, approval_request, reason, grant_root) .await; await_approval_with_cancel( async move { decision_rx.await.unwrap_or_default() }, @@ -684,12 +684,18 @@ async fn maybe_auto_review_mcp_request_user_input( &invocation.tool, ) .await; + let approval_request = build_mcp_tool_approval_request( + &event.call_id, + &invocation.tool, + &invocation, + metadata.as_ref(), + ); let review_cancel = cancel_token.child_token(); let review_rx = spawn_approval_request_review( Arc::clone(parent_session), Arc::clone(parent_ctx), new_guardian_review_id(), - build_guardian_mcp_tool_review_request(&event.call_id, &invocation, metadata.as_ref()), + approval_request.clone(), /*retry_reason*/ None, GuardianApprovalRequestSource::DelegatedSubagent, review_cancel.clone(), @@ -702,32 +708,7 @@ async fn maybe_auto_review_mcp_request_user_input( Some(&review_cancel), ) .await; - let selected_label = match decision { - ReviewDecision::ApprovedForSession => question - .options - .as_ref() - .and_then(|options| { - options - .iter() - .find(|option| option.label == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION) - }) - .map(|option| option.label.clone()) - .unwrap_or_else(|| MCP_TOOL_APPROVAL_ACCEPT.to_string()), - ReviewDecision::Approved - | ReviewDecision::ApprovedExecpolicyAmendment { .. } - | ReviewDecision::NetworkPolicyAmendment { .. } => MCP_TOOL_APPROVAL_ACCEPT.to_string(), - ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => { - MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string() - } - }; - Some(RequestUserInputResponse { - answers: HashMap::from([( - question.id.clone(), - codex_protocol::request_user_input::RequestUserInputAnswer { - answers: vec![selected_label], - }, - )]), - }) + mcp_tool_approval_compat_response(&approval_request, question, decision) } async fn handle_request_permissions( diff --git a/codex-rs/core/src/codex_delegate_tests.rs b/codex-rs/core/src/codex_delegate_tests.rs index 84224ea2d528..fa86d97d2eb9 100644 --- a/codex-rs/core/src/codex_delegate_tests.rs +++ b/codex-rs/core/src/codex_delegate_tests.rs @@ -1,14 +1,15 @@ use super::*; use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC; -use crate::mcp_tool_call::MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX; use async_channel::bounded; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::models::NetworkPermissions; use codex_protocol::models::ResponseItem; use codex_protocol::protocol::AgentStatus; +use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::ExecApprovalRequestEvent; +use codex_protocol::protocol::FileChange; use codex_protocol::protocol::GuardianAssessmentAction; use codex_protocol::protocol::GuardianAssessmentStatus; use codex_protocol::protocol::GuardianCommandSource; @@ -384,6 +385,91 @@ async fn handle_exec_approval_uses_call_id_for_guardian_review_and_approval_id_f ); } +#[tokio::test] +async fn handle_patch_approval_uses_tool_call_id_for_round_trip() { + let (parent_session, parent_ctx, rx_events) = + crate::session::tests::make_session_and_context_with_rx().await; + *parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default()); + + let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY); + let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY); + let (_agent_status_tx, agent_status) = watch::channel(AgentStatus::PendingInit); + let codex = Arc::new(Codex { + tx_sub, + rx_event: rx_events_child, + agent_status, + session: Arc::clone(&parent_session), + session_loop_termination: completed_session_loop_termination(), + }); + + let call_id = "patch-call-1".to_string(); + let changes = HashMap::from([( + std::path::PathBuf::from("file.txt"), + FileChange::Update { + unified_diff: "@@ -1 +1 @@\n-old\n+new\n".to_string(), + move_path: None, + }, + )]); + let expected_changes = changes.clone(); + let cancel_token = CancellationToken::new(); + + let handle = tokio::spawn({ + let codex = Arc::clone(&codex); + let parent_session = Arc::clone(&parent_session); + let parent_ctx = Arc::clone(&parent_ctx); + let cancel_token = cancel_token.clone(); + let call_id = call_id.clone(); + async move { + handle_patch_approval( + codex.as_ref(), + "child-turn-1".to_string(), + &parent_session, + &parent_ctx, + ApplyPatchApprovalRequestEvent { + call_id, + turn_id: "child-turn-1".to_string(), + changes, + reason: Some("needs write".to_string()), + grant_root: None, + }, + &cancel_token, + ) + .await; + } + }); + + let request_event = timeout(Duration::from_secs(1), rx_events.recv()) + .await + .expect("patch approval event timed out") + .expect("patch approval event missing"); + let EventMsg::ApplyPatchApprovalRequest(request) = request_event.msg else { + panic!("expected ApplyPatchApprovalRequest event"); + }; + assert_eq!(request.call_id, call_id.clone()); + assert_eq!(request.changes, expected_changes); + + parent_session + .notify_approval(&call_id, ReviewDecision::Approved) + .await; + + timeout(Duration::from_secs(1), handle) + .await + .expect("handle_patch_approval hung") + .expect("handle_patch_approval join error"); + + let submission = timeout(Duration::from_secs(1), rx_sub.recv()) + .await + .expect("patch approval response timed out") + .expect("patch approval response missing"); + assert_eq!( + submission.op, + Op::PatchApproval { + id: call_id, + decision: ReviewDecision::Approved, + } + ); +} + #[tokio::test] async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() { let (parent_session, parent_ctx, _rx_events) = @@ -417,7 +503,7 @@ async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() { call_id: "call-1".to_string(), turn_id: "child-turn-1".to_string(), questions: vec![RequestUserInputQuestion { - id: format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_call-1"), + id: "mcp_tool_call_approval_call-1".to_string(), header: "Approve app tool call?".to_string(), question: "Allow this app tool?".to_string(), is_other: false, @@ -433,7 +519,7 @@ async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() { response, Some(RequestUserInputResponse { answers: HashMap::from([( - format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_call-1"), + "mcp_tool_call_approval_call-1".to_string(), RequestUserInputAnswer { answers: vec![MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string()], }, diff --git a/codex-rs/core/src/guardian/approval_request.rs b/codex-rs/core/src/guardian/approval_request.rs index fba227834af2..8deb4720c08a 100644 --- a/codex-rs/core/src/guardian/approval_request.rs +++ b/codex-rs/core/src/guardian/approval_request.rs @@ -1,23 +1,41 @@ +use std::collections::HashMap; use std::path::Path; +use std::path::PathBuf; use codex_analytics::GuardianReviewedAction; +use codex_protocol::approvals::ExecApprovalRequestEvent; use codex_protocol::approvals::GuardianAssessmentAction; use codex_protocol::approvals::GuardianCommandSource; +use codex_protocol::approvals::NetworkApprovalContext; use codex_protocol::approvals::NetworkApprovalProtocol; +use codex_protocol::approvals::NetworkPolicyAmendment; use codex_protocol::models::AdditionalPermissionProfile; +use codex_protocol::protocol::ApplyPatchApprovalRequestEvent; +use codex_protocol::protocol::ExecPolicyAmendment; +use codex_protocol::protocol::FileChange; +use codex_protocol::protocol::ReviewDecision; use codex_protocol::request_permissions::RequestPermissionProfile; +use codex_protocol::request_permissions::RequestPermissionsEvent; use codex_utils_absolute_path::AbsolutePathBuf; use serde::Serialize; use serde_json::Value; use super::GUARDIAN_MAX_ACTION_STRING_TOKENS; use super::prompt::guardian_truncate_text; +use crate::tools::hook_names::HookToolName; +use crate::tools::sandboxing::PermissionRequestPayload; +/// Canonical description of an approval-worthy action in core. +/// +/// This type should describe the action being reviewed exactly once, with +/// guardian review, approval hooks, and user-prompt transports deriving their +/// own projections from it. #[derive(Debug, Clone, PartialEq)] pub(crate) enum GuardianApprovalRequest { Shell { id: String, command: Vec, + hook_command: String, cwd: AbsolutePathBuf, sandbox_permissions: crate::sandboxing::SandboxPermissions, additional_permissions: Option, @@ -26,6 +44,7 @@ pub(crate) enum GuardianApprovalRequest { ExecCommand { id: String, command: Vec, + hook_command: String, cwd: AbsolutePathBuf, sandbox_permissions: crate::sandboxing::SandboxPermissions, additional_permissions: Option, @@ -45,12 +64,14 @@ pub(crate) enum GuardianApprovalRequest { id: String, cwd: AbsolutePathBuf, files: Vec, + changes: HashMap, patch: String, }, NetworkAccess { id: String, turn_id: String, target: String, + hook_command: String, host: String, protocol: NetworkApprovalProtocol, port: u16, @@ -60,6 +81,7 @@ pub(crate) enum GuardianApprovalRequest { id: String, server: String, tool_name: String, + hook_tool_name: String, arguments: Option, connector_id: Option, connector_name: Option, @@ -73,9 +95,222 @@ pub(crate) enum GuardianApprovalRequest { turn_id: String, reason: Option, permissions: RequestPermissionProfile, + cwd: AbsolutePathBuf, }, } +impl GuardianApprovalRequest { + pub(crate) fn permission_request_payload(&self) -> Option { + match self { + Self::Shell { + hook_command, + justification, + .. + } + | Self::ExecCommand { + hook_command, + justification, + .. + } => Some(PermissionRequestPayload::bash( + hook_command.clone(), + justification.clone(), + )), + #[cfg(unix)] + Self::Execve { program, argv, .. } => { + let mut command = vec![program.clone()]; + if argv.len() > 1 { + command.extend_from_slice(&argv[1..]); + } + Some(PermissionRequestPayload::bash( + codex_shell_command::parse_command::shlex_join(&command), + /*description*/ None, + )) + } + Self::ApplyPatch { patch, .. } => Some(PermissionRequestPayload { + tool_name: HookToolName::apply_patch(), + tool_input: serde_json::json!({ "command": patch }), + }), + Self::NetworkAccess { + target, + hook_command, + .. + } => Some(PermissionRequestPayload::bash( + hook_command.clone(), + Some(format!("network-access {target}")), + )), + Self::McpToolCall { + hook_tool_name, + arguments, + .. + } => Some(PermissionRequestPayload { + tool_name: HookToolName::new(hook_tool_name.clone()), + tool_input: arguments + .clone() + .unwrap_or_else(|| Value::Object(serde_json::Map::new())), + }), + Self::RequestPermissions { .. } => None, + } + } + + #[allow(clippy::too_many_arguments)] + pub(crate) fn exec_approval_event( + &self, + turn_id: String, + approval_id: Option, + reason: Option, + network_approval_context: Option, + proposed_execpolicy_amendment: Option, + proposed_network_policy_amendments: Option>, + available_decisions: Option>, + fallback_cwd: Option, + ) -> Option { + match self { + Self::Shell { + id, + command, + cwd, + additional_permissions, + .. + } + | Self::ExecCommand { + id, + command, + cwd, + additional_permissions, + .. + } => Some(ExecApprovalRequestEvent { + call_id: id.clone(), + approval_id, + turn_id, + command: command.clone(), + cwd: cwd.clone(), + reason, + network_approval_context, + proposed_execpolicy_amendment, + proposed_network_policy_amendments, + additional_permissions: additional_permissions.clone(), + available_decisions, + parsed_cmd: codex_shell_command::parse_command::parse_command(command), + }), + #[cfg(unix)] + Self::Execve { + id, + argv, + cwd, + additional_permissions, + .. + } => Some(ExecApprovalRequestEvent { + call_id: id.clone(), + approval_id, + turn_id, + command: argv.clone(), + cwd: cwd.clone(), + reason, + network_approval_context, + proposed_execpolicy_amendment, + proposed_network_policy_amendments, + additional_permissions: additional_permissions.clone(), + available_decisions, + parsed_cmd: codex_shell_command::parse_command::parse_command(argv), + }), + Self::NetworkAccess { + id, + turn_id, + target, + host, + protocol, + .. + } => { + let command = vec!["network-access".to_string(), target.clone()]; + let cwd = fallback_cwd?; + let network_approval_context = Some(NetworkApprovalContext { + host: host.clone(), + protocol: *protocol, + }); + let proposed_network_policy_amendments = proposed_network_policy_amendments + .or_else(|| { + Some(vec![ + NetworkPolicyAmendment { + host: host.clone(), + action: codex_protocol::approvals::NetworkPolicyRuleAction::Allow, + }, + NetworkPolicyAmendment { + host: host.clone(), + action: codex_protocol::approvals::NetworkPolicyRuleAction::Deny, + }, + ]) + }); + Some(ExecApprovalRequestEvent { + call_id: id.clone(), + approval_id, + turn_id: turn_id.clone(), + command: command.clone(), + cwd, + reason, + network_approval_context, + proposed_execpolicy_amendment: None, + proposed_network_policy_amendments, + additional_permissions: None, + available_decisions, + parsed_cmd: codex_shell_command::parse_command::parse_command(&command), + }) + } + Self::ApplyPatch { .. } + | Self::McpToolCall { .. } + | Self::RequestPermissions { .. } => None, + } + } + + pub(crate) fn apply_patch_approval_event( + &self, + turn_id: String, + reason: Option, + grant_root: Option, + ) -> Option { + match self { + Self::ApplyPatch { id, changes, .. } => Some(ApplyPatchApprovalRequestEvent { + call_id: id.clone(), + turn_id, + changes: changes.clone(), + reason, + grant_root, + }), + Self::Shell { .. } + | Self::ExecCommand { .. } + | Self::NetworkAccess { .. } + | Self::McpToolCall { .. } + | Self::RequestPermissions { .. } => None, + #[cfg(unix)] + Self::Execve { .. } => None, + } + } + + pub(crate) fn request_permissions_event(&self) -> Option { + match self { + Self::RequestPermissions { + id, + turn_id, + reason, + permissions, + cwd, + } => Some(RequestPermissionsEvent { + call_id: id.clone(), + turn_id: turn_id.clone(), + reason: reason.clone(), + permissions: permissions.clone(), + cwd: Some(cwd.clone()), + }), + Self::Shell { .. } + | Self::ExecCommand { .. } + | Self::ApplyPatch { .. } + | Self::NetworkAccess { .. } + | Self::McpToolCall { .. } => None, + #[cfg(unix)] + Self::Execve { .. } => None, + } + } +} + #[derive(Debug, Clone, PartialEq, Serialize)] #[serde(rename_all = "camelCase")] pub(crate) struct GuardianNetworkAccessTrigger { @@ -267,6 +502,7 @@ pub(crate) fn guardian_approval_request_to_json( sandbox_permissions, additional_permissions, justification, + .. } => serialize_command_guardian_action( "shell", command, @@ -284,6 +520,7 @@ pub(crate) fn guardian_approval_request_to_json( additional_permissions, justification, tty, + .. } => serialize_command_guardian_action( "exec_command", command, @@ -312,6 +549,7 @@ pub(crate) fn guardian_approval_request_to_json( id: _, cwd, files, + changes: _, patch, } => Ok(serde_json::json!({ "tool": "apply_patch", @@ -327,6 +565,7 @@ pub(crate) fn guardian_approval_request_to_json( protocol, port, trigger, + .. } => serialize_guardian_action(NetworkAccessApprovalAction { tool: "network_access", target, @@ -346,6 +585,7 @@ pub(crate) fn guardian_approval_request_to_json( tool_title, tool_description, annotations, + .. } => serialize_guardian_action(McpToolCallApprovalAction { tool: "mcp_tool_call", server, @@ -363,6 +603,7 @@ pub(crate) fn guardian_approval_request_to_json( turn_id, reason, permissions, + .. } => serialize_guardian_action(RequestPermissionsApprovalAction { tool: "request_permissions", turn_id, @@ -409,6 +650,7 @@ pub(crate) fn guardian_assessment_action( protocol, port, trigger: _trigger, + .. } => GuardianAssessmentAction::NetworkAccess { target: target.clone(), host: host.clone(), @@ -539,3 +781,180 @@ pub(crate) fn format_guardian_action_pretty( truncated, }) } + +#[cfg(test)] +mod tests { + use super::*; + use codex_protocol::protocol::FileChange; + use codex_utils_absolute_path::test_support::PathBufExt; + use codex_utils_absolute_path::test_support::test_path_buf; + use pretty_assertions::assert_eq; + + #[test] + fn exec_approval_event_is_projected_from_shell_request() { + let request = GuardianApprovalRequest::Shell { + id: "call-1".to_string(), + command: vec!["echo".to_string(), "hi".to_string()], + hook_command: "echo hi".to_string(), + cwd: test_path_buf("/tmp").abs(), + sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, + additional_permissions: None, + justification: Some("because".to_string()), + }; + + let event = request + .exec_approval_event( + "turn-1".to_string(), + Some("approval-1".to_string()), + Some("retry".to_string()), + /*network_approval_context*/ None, + /*proposed_execpolicy_amendment*/ None, + /*proposed_network_policy_amendments*/ None, + Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), + /*fallback_cwd*/ None, + ) + .expect("exec approval event"); + + assert_eq!(event.call_id, "call-1"); + assert_eq!(event.approval_id.as_deref(), Some("approval-1")); + assert_eq!(event.turn_id, "turn-1"); + assert_eq!(event.command, vec!["echo".to_string(), "hi".to_string()]); + assert_eq!(event.reason.as_deref(), Some("retry")); + assert_eq!( + event.available_decisions, + Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]) + ); + } + + #[test] + fn apply_patch_approval_event_is_projected_from_request() { + let path = test_path_buf("/tmp/file.txt"); + let abs_path = path.abs(); + let request = GuardianApprovalRequest::ApplyPatch { + id: "call-1".to_string(), + cwd: test_path_buf("/tmp").abs(), + files: vec![abs_path], + changes: HashMap::from([( + path.clone(), + FileChange::Add { + content: "hello".to_string(), + }, + )]), + patch: "*** Begin Patch".to_string(), + }; + + let event = request + .apply_patch_approval_event( + "turn-1".to_string(), + Some("needs write".to_string()), + /*grant_root*/ None, + ) + .expect("apply_patch approval event"); + + assert_eq!(event.call_id, "call-1"); + assert_eq!(event.turn_id, "turn-1"); + assert_eq!(event.reason.as_deref(), Some("needs write")); + assert_eq!( + event.changes, + HashMap::from([( + path, + FileChange::Add { + content: "hello".to_string(), + }, + )]) + ); + } + + #[test] + fn request_permissions_event_is_projected_from_request() { + let request = GuardianApprovalRequest::RequestPermissions { + id: "call-1".to_string(), + turn_id: "turn-1".to_string(), + reason: Some("need outbound network".to_string()), + permissions: RequestPermissionProfile { + network: Some(codex_protocol::models::NetworkPermissions { + enabled: Some(true), + }), + file_system: None, + }, + cwd: test_path_buf("/tmp").abs(), + }; + + let event = request + .request_permissions_event() + .expect("request_permissions event"); + + assert_eq!(event.call_id, "call-1"); + assert_eq!(event.turn_id, "turn-1"); + assert_eq!(event.reason.as_deref(), Some("need outbound network")); + assert_eq!( + event.permissions, + RequestPermissionProfile { + network: Some(codex_protocol::models::NetworkPermissions { + enabled: Some(true), + }), + file_system: None, + } + ); + assert_eq!(event.cwd, Some(test_path_buf("/tmp").abs())); + } + + #[test] + fn network_exec_approval_event_is_projected_from_request() { + let request = GuardianApprovalRequest::NetworkAccess { + id: "network-1".to_string(), + turn_id: "turn-1".to_string(), + target: "https://example.com:443".to_string(), + hook_command: "curl https://example.com".to_string(), + host: "example.com".to_string(), + protocol: NetworkApprovalProtocol::Https, + port: 443, + trigger: None, + }; + + let event = request + .exec_approval_event( + "ignored-turn".to_string(), + /*approval_id*/ None, + Some("need network".to_string()), + /*network_approval_context*/ None, + /*proposed_execpolicy_amendment*/ None, + /*proposed_network_policy_amendments*/ None, + /*available_decisions*/ None, + Some(test_path_buf("/tmp").abs()), + ) + .expect("network exec approval event"); + + assert_eq!(event.call_id, "network-1"); + assert_eq!(event.turn_id, "turn-1"); + assert_eq!( + event.command, + vec![ + "network-access".to_string(), + "https://example.com:443".to_string() + ] + ); + assert_eq!(event.cwd, test_path_buf("/tmp").abs()); + assert_eq!(event.reason.as_deref(), Some("need network")); + assert_eq!( + event.network_approval_context, + Some(NetworkApprovalContext { + host: "example.com".to_string(), + protocol: NetworkApprovalProtocol::Https, + }) + ); + assert_eq!( + event.proposed_network_policy_amendments, + Some(vec![ + NetworkPolicyAmendment { + host: "example.com".to_string(), + action: codex_protocol::approvals::NetworkPolicyRuleAction::Allow, + }, + NetworkPolicyAmendment { + host: "example.com".to_string(), + action: codex_protocol::approvals::NetworkPolicyRuleAction::Deny, + }, + ]) + ); + } +} diff --git a/codex-rs/core/src/guardian/review_session.rs b/codex-rs/core/src/guardian/review_session.rs index 6fd50219d88c..a9d3aea48a71 100644 --- a/codex-rs/core/src/guardian/review_session.rs +++ b/codex-rs/core/src/guardian/review_session.rs @@ -1104,6 +1104,7 @@ mod tests { request: GuardianApprovalRequest::Shell { id: "shell-1".to_string(), command: vec!["git".to_string(), "status".to_string()], + hook_command: "git status".to_string(), cwd, sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, diff --git a/codex-rs/core/src/guardian/tests.rs b/codex-rs/core/src/guardian/tests.rs index 78362b6f8985..14e5a0ddf50d 100644 --- a/codex-rs/core/src/guardian/tests.rs +++ b/codex-rs/core/src/guardian/tests.rs @@ -1,3 +1,5 @@ +use std::collections::HashMap; + use super::*; use crate::config::Config; use crate::config::ConfigOverrides; @@ -60,7 +62,6 @@ use insta::Settings; use insta::assert_snapshot; use pretty_assertions::assert_eq; use std::collections::BTreeMap; -use std::collections::HashMap; use std::sync::Arc; use std::time::Duration; use tempfile::TempDir; @@ -315,6 +316,7 @@ async fn build_guardian_prompt_full_mode_preserves_initial_review_format() -> an GuardianApprovalRequest::Shell { id: "shell-1".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -369,6 +371,7 @@ async fn build_guardian_prompt_delta_mode_preserves_original_numbering() -> anyh GuardianApprovalRequest::Shell { id: "shell-2".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -407,6 +410,7 @@ async fn build_guardian_prompt_delta_mode_handles_empty_delta() -> anyhow::Resul GuardianApprovalRequest::Shell { id: "shell-2".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -442,6 +446,7 @@ async fn build_guardian_prompt_stale_delta_cursor_falls_back_to_full_prompt() -> GuardianApprovalRequest::Shell { id: "shell-3".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -523,6 +528,7 @@ async fn build_guardian_prompt_stale_delta_version_falls_back_to_full_prompt() - GuardianApprovalRequest::Shell { id: "shell-4".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -688,6 +694,7 @@ fn format_guardian_action_pretty_truncates_large_string_fields() -> serde_json:: id: "patch-1".to_string(), cwd: test_path_buf("/tmp").abs(), files: Vec::new(), + changes: HashMap::new(), patch: patch.clone(), }; @@ -707,6 +714,7 @@ fn format_guardian_action_pretty_reports_no_truncation_for_small_payload() -> se id: "patch-1".to_string(), cwd: test_path_buf("/tmp").abs(), files: Vec::new(), + changes: HashMap::new(), patch: "line\n".to_string(), }; @@ -723,6 +731,7 @@ fn guardian_approval_request_to_json_renders_mcp_tool_call_shape() -> serde_json id: "call-1".to_string(), server: "mcp_server".to_string(), tool_name: "browser_navigate".to_string(), + hook_tool_name: "browser_navigate".to_string(), arguments: Some(serde_json::json!({ "url": "https://example.com", })), @@ -765,6 +774,7 @@ fn guardian_approval_request_to_json_renders_network_access_trigger() -> serde_j id: "network-1".to_string(), turn_id: "turn-1".to_string(), target: "https://example.com:443".to_string(), + hook_command: "curl https://example.com".to_string(), host: "example.com".to_string(), protocol: NetworkApprovalProtocol::Https, port: 443, @@ -815,6 +825,7 @@ async fn build_guardian_prompt_items_explains_network_access_review_scope() -> a id: "network-1".to_string(), turn_id: "turn-1".to_string(), target: "https://example.com:443".to_string(), + hook_command: "curl https://example.com".to_string(), host: "example.com".to_string(), protocol: NetworkApprovalProtocol::Https, port: 443, @@ -879,6 +890,7 @@ fn guardian_assessment_action_redacts_apply_patch_patch_text() { id: "patch-1".to_string(), cwd: cwd.clone(), files: vec![file.clone()], + changes: HashMap::new(), patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+secret\n*** End Patch" .to_string(), }; @@ -899,6 +911,7 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() { id: "network-1".to_string(), turn_id: "owner-turn".to_string(), target: "https://example.com:443".to_string(), + hook_command: "network-access https://example.com:443".to_string(), host: "example.com".to_string(), protocol: NetworkApprovalProtocol::Https, port: 443, @@ -908,6 +921,7 @@ fn guardian_request_turn_id_prefers_network_access_owner_turn() { id: "patch-1".to_string(), cwd: test_path_buf("/tmp").abs(), files: vec![test_path_buf("/tmp/guardian.txt").abs()], + changes: HashMap::new(), patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+hello\n*** End Patch" .to_string(), }; @@ -928,6 +942,7 @@ fn guardian_request_target_item_id_omits_network_access_trigger_call_id() { id: "network-1".to_string(), turn_id: "owner-turn".to_string(), target: "https://example.com:443".to_string(), + hook_command: "network-access https://example.com:443".to_string(), host: "example.com".to_string(), protocol: NetworkApprovalProtocol::Https, port: 443, @@ -960,6 +975,7 @@ async fn cancelled_guardian_review_emits_terminal_abort_without_warning() { id: "patch-1".to_string(), cwd: test_path_buf("/tmp").abs(), files: vec![test_path_buf("/tmp/guardian.txt").abs()], + changes: HashMap::new(), patch: "*** Begin Patch\n*** Update File: guardian.txt\n@@\n+hello\n*** End Patch" .to_string(), }, @@ -1253,6 +1269,7 @@ async fn guardian_review_request_layout_matches_model_visible_request_snapshot() "origin".to_string(), "guardian-approval-mvp".to_string(), ], + hook_command: "git push origin guardian-approval-mvp".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1358,6 +1375,7 @@ async fn build_guardian_prompt_items_includes_parent_session_id() -> anyhow::Res GuardianApprovalRequest::Shell { id: "shell-1".to_string(), command: vec!["git".to_string(), "status".to_string()], + hook_command: "git status".to_string(), cwd: test_path_buf("/repo").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1432,6 +1450,7 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow: let first_request = GuardianApprovalRequest::Shell { id: "shell-1".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1476,6 +1495,7 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow: "push".to_string(), "--force-with-lease".to_string(), ], + hook_command: "git push --force-with-lease".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1516,6 +1536,7 @@ async fn guardian_reuses_prompt_cache_key_and_appends_prior_reviews() -> anyhow: let third_request = GuardianApprovalRequest::Shell { id: "shell-3".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1711,6 +1732,7 @@ async fn guardian_reused_trunk_ignores_stale_prior_turn_completion() -> anyhow:: GuardianApprovalRequest::Shell { id: "shell-1".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1753,6 +1775,7 @@ async fn guardian_reused_trunk_ignores_stale_prior_turn_completion() -> anyhow:: GuardianApprovalRequest::Shell { id: "shell-2".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1832,6 +1855,7 @@ async fn guardian_review_surfaces_responses_api_errors_in_rejection_reason() -> GuardianApprovalRequest::Shell { id: "shell-guardian-error".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -1966,6 +1990,7 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a let initial_request = GuardianApprovalRequest::Shell { id: "shell-guardian-1".to_string(), command: vec!["git".to_string(), "status".to_string()], + hook_command: "git status".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -2009,6 +2034,7 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a let second_request = GuardianApprovalRequest::Shell { id: "shell-guardian-2".to_string(), command: vec!["git".to_string(), "diff".to_string()], + hook_command: "git diff".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, @@ -2017,6 +2043,7 @@ async fn guardian_parallel_reviews_fork_from_last_committed_trunk_history() -> a let third_request = GuardianApprovalRequest::Shell { id: "shell-guardian-3".to_string(), command: vec!["git".to_string(), "push".to_string()], + hook_command: "git push".to_string(), cwd: test_path_buf("/repo/codex-rs/core").abs(), sandbox_permissions: crate::sandboxing::SandboxPermissions::UseDefault, additional_permissions: None, diff --git a/codex-rs/core/src/mcp_tool_call.rs b/codex-rs/core/src/mcp_tool_call.rs index 85fc939ba9f8..1b1ddbf1eab7 100644 --- a/codex-rs/core/src/mcp_tool_call.rs +++ b/codex-rs/core/src/mcp_tool_call.rs @@ -31,8 +31,6 @@ 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::turn_context::TurnContext; -use crate::tools::hook_names::HookToolName; -use crate::tools::sandboxing::PermissionRequestPayload; use codex_analytics::AppInvocation; use codex_analytics::InvocationType; use codex_analytics::build_track_events_context; @@ -885,7 +883,7 @@ struct McpToolApprovalPromptOptions { struct McpToolApprovalElicitationRequest<'a> { server: &'a str, - metadata: Option<&'a McpToolApprovalMetadata>, + approval_request: &'a GuardianApprovalRequest, tool_params: Option<&'a serde_json::Value>, tool_params_display: Option<&'a [RenderedMcpToolApprovalParam]>, question: RequestUserInputQuestion, @@ -893,6 +891,14 @@ struct McpToolApprovalElicitationRequest<'a> { prompt_options: McpToolApprovalPromptOptions, } +#[derive(Clone, Debug, PartialEq)] +struct McpToolApprovalPrompt { + question: RequestUserInputQuestion, + message_override: Option, + tool_params: Option, + tool_params_display: Option>, +} + pub(crate) const MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX: &str = "mcp_tool_call_approval"; pub(crate) const MCP_TOOL_APPROVAL_ACCEPT: &str = "Allow"; pub(crate) const MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION: &str = "Allow for this session"; @@ -917,6 +923,7 @@ const MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: &str = "tool_title"; const MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: &str = "tool_description"; const MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: &str = "tool_params"; const MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY: &str = "tool_params_display"; + const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_DEFAULT: &str = "mcp_tool_call__default"; const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_ALWAYS_ALLOW: &str = "mcp_tool_call__always_allow"; @@ -933,6 +940,105 @@ struct McpToolApprovalKey { tool_name: String, } +/// Builds the user-facing MCP approval prompt from the canonical approval request. +fn mcp_tool_approval_prompt( + approval_request: &GuardianApprovalRequest, + question_id: String, + prompt_options: McpToolApprovalPromptOptions, + monitor_reason: Option<&str>, +) -> Option { + let GuardianApprovalRequest::McpToolCall { + server, + tool_name, + arguments, + connector_id, + connector_name, + tool_title, + .. + } = approval_request + else { + return None; + }; + + let rendered_template = render_mcp_tool_approval_template( + 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(arguments.as_ref())); + let question_override = rendered_template + .as_ref() + .map(|rendered_template| rendered_template.question.as_str()); + let mut question = build_mcp_tool_approval_question( + question_id, + server, + tool_name, + connector_name.as_deref(), + prompt_options, + question_override, + ); + question.question = mcp_tool_approval_question_text(question.question, monitor_reason); + + Some(McpToolApprovalPrompt { + question, + message_override: rendered_template.as_ref().and_then(|rendered_template| { + monitor_reason + .is_none() + .then_some(rendered_template.elicitation_message.clone()) + }), + tool_params: rendered_template + .as_ref() + .and_then(|rendered_template| rendered_template.tool_params.clone()) + .or_else(|| arguments.clone()), + tool_params_display, + }) +} + +/// Converts a guardian MCP approval decision into the legacy RequestUserInput +/// response shape used by delegated compatibility flows. +pub(crate) fn mcp_tool_approval_compat_response( + approval_request: &GuardianApprovalRequest, + question: &RequestUserInputQuestion, + decision: ReviewDecision, +) -> Option { + let GuardianApprovalRequest::McpToolCall { .. } = approval_request else { + return None; + }; + + let selected_label = match decision { + ReviewDecision::ApprovedForSession => question + .options + .as_ref() + .and_then(|options| { + options + .iter() + .find(|option| option.label == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION) + }) + .map(|option| option.label.clone()) + .unwrap_or_else(|| MCP_TOOL_APPROVAL_ACCEPT.to_string()), + ReviewDecision::Approved + | ReviewDecision::ApprovedExecpolicyAmendment { .. } + | ReviewDecision::NetworkPolicyAmendment { .. } => MCP_TOOL_APPROVAL_ACCEPT.to_string(), + ReviewDecision::Denied | ReviewDecision::TimedOut | ReviewDecision::Abort => { + MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string() + } + }; + + Some(RequestUserInputResponse { + answers: HashMap::from([( + question.id.clone(), + RequestUserInputAnswer { + answers: vec![selected_label], + }, + )]), + }) +} + fn mcp_tool_approval_prompt_options( session_approval_key: Option<&McpToolApprovalKey>, persistent_approval_key: Option<&McpToolApprovalKey>, @@ -1005,18 +1111,15 @@ 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())), - }, - ) + 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) => { @@ -1041,7 +1144,7 @@ async fn maybe_request_mcp_tool_approval( sess, turn_context, review_id.clone(), - build_guardian_mcp_tool_review_request(call_id, invocation, metadata), + approval_request, monitor_reason.clone(), ) .await; @@ -1063,50 +1166,26 @@ async fn maybe_request_mcp_tool_approval( tool_call_mcp_elicitation_enabled, ); let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}"); - let rendered_template = render_mcp_tool_approval_template( - &invocation.server, - metadata.and_then(|metadata| metadata.connector_id.as_deref()), - metadata.and_then(|metadata| metadata.connector_name.as_deref()), - metadata.and_then(|metadata| metadata.tool_title.as_deref()), - invocation.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())); - let mut question = build_mcp_tool_approval_question( + let Some(prompt) = mcp_tool_approval_prompt( + &approval_request, question_id.clone(), - &invocation.server, - &invocation.tool, - metadata.and_then(|metadata| metadata.connector_name.as_deref()), prompt_options, - rendered_template - .as_ref() - .map(|rendered_template| rendered_template.question.as_str()), - ); - question.question = - mcp_tool_approval_question_text(question.question, monitor_reason.as_deref()); + monitor_reason.as_deref(), + ) else { + unreachable!("MCP approvals always project an MCP approval prompt"); + }; if tool_call_mcp_elicitation_enabled { - let request_id = rmcp::model::RequestId::String( - format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}").into(), - ); + let request_id = rmcp::model::RequestId::String(question_id.clone().into()); let params = build_mcp_tool_approval_elicitation_request( sess.as_ref(), turn_context.as_ref(), McpToolApprovalElicitationRequest { server: &invocation.server, - metadata, - tool_params: rendered_template - .as_ref() - .and_then(|rendered_template| rendered_template.tool_params.as_ref()) - .or(invocation.arguments.as_ref()), - tool_params_display: tool_params_display.as_deref(), - question, - message_override: rendered_template.as_ref().and_then(|rendered_template| { - monitor_reason - .is_none() - .then_some(rendered_template.elicitation_message.as_str()) - }), + approval_request: &approval_request, + tool_params: prompt.tool_params.as_ref(), + tool_params_display: prompt.tool_params_display.as_deref(), + question: prompt.question, + message_override: prompt.message_override.as_deref(), prompt_options, }, ); @@ -1128,7 +1207,7 @@ async fn maybe_request_mcp_tool_approval( } let args = RequestUserInputArgs { - questions: vec![question], + questions: vec![prompt.question], }; let response = sess .request_user_input(turn_context.as_ref(), call_id.to_string(), args) @@ -1169,7 +1248,8 @@ fn prepare_arc_request_action( invocation: &McpInvocation, metadata: Option<&McpToolApprovalMetadata>, ) -> serde_json::Value { - let request = build_guardian_mcp_tool_review_request("arc-monitor", invocation, metadata); + let request = + build_mcp_tool_approval_request("arc-monitor", &invocation.tool, invocation, metadata); match guardian_approval_request_to_json(&request) { Ok(action) => action, Err(error) => { @@ -1208,8 +1288,9 @@ fn persistent_mcp_tool_approval_key( session_mcp_tool_approval_key(invocation, metadata, approval_mode) } -pub(crate) fn build_guardian_mcp_tool_review_request( +pub(crate) fn build_mcp_tool_approval_request( call_id: &str, + hook_tool_name: &str, invocation: &McpInvocation, metadata: Option<&McpToolApprovalMetadata>, ) -> GuardianApprovalRequest { @@ -1217,6 +1298,7 @@ pub(crate) fn build_guardian_mcp_tool_review_request( id: call_id.to_string(), server: invocation.server.clone(), tool_name: invocation.tool.clone(), + hook_tool_name: hook_tool_name.to_string(), arguments: invocation.arguments.clone(), connector_id: metadata.and_then(|metadata| metadata.connector_id.clone()), connector_name: metadata.and_then(|metadata| metadata.connector_name.clone()), @@ -1489,8 +1571,7 @@ fn build_mcp_tool_approval_elicitation_request( server_name: request.server.to_string(), request: McpServerElicitationRequest::Form { meta: build_mcp_tool_approval_elicitation_meta( - request.server, - request.metadata, + request.approval_request, request.tool_params, request.tool_params_display, request.prompt_options, @@ -1507,16 +1588,28 @@ fn build_mcp_tool_approval_elicitation_request( } fn build_mcp_tool_approval_elicitation_meta( - server: &str, - metadata: Option<&McpToolApprovalMetadata>, - tool_params: Option<&serde_json::Value>, + approval_request: &GuardianApprovalRequest, + tool_params: Option<&JsonValue>, tool_params_display: Option<&[RenderedMcpToolApprovalParam]>, prompt_options: McpToolApprovalPromptOptions, -) -> Option { +) -> Option { + let GuardianApprovalRequest::McpToolCall { + server, + connector_id, + connector_name, + connector_description, + tool_title, + tool_description, + .. + } = approval_request + else { + return None; + }; + let mut meta = serde_json::Map::new(); meta.insert( MCP_TOOL_APPROVAL_KIND_KEY.to_string(), - serde_json::Value::String(MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL.to_string()), + JsonValue::String(MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL.to_string()), ); match ( prompt_options.allow_session_remember, @@ -1534,57 +1627,53 @@ fn build_mcp_tool_approval_elicitation_meta( (true, false) => { meta.insert( MCP_TOOL_APPROVAL_PERSIST_KEY.to_string(), - serde_json::Value::String(MCP_TOOL_APPROVAL_PERSIST_SESSION.to_string()), + JsonValue::String(MCP_TOOL_APPROVAL_PERSIST_SESSION.to_string()), ); } (false, true) => { meta.insert( MCP_TOOL_APPROVAL_PERSIST_KEY.to_string(), - serde_json::Value::String(MCP_TOOL_APPROVAL_PERSIST_ALWAYS.to_string()), + JsonValue::String(MCP_TOOL_APPROVAL_PERSIST_ALWAYS.to_string()), ); } (false, false) => {} } - if let Some(metadata) = metadata { - if let Some(tool_title) = metadata.tool_title.as_ref() { + if let Some(tool_title) = tool_title.as_ref() { + meta.insert( + MCP_TOOL_APPROVAL_TOOL_TITLE_KEY.to_string(), + JsonValue::String(tool_title.clone()), + ); + } + if let Some(tool_description) = tool_description.as_ref() { + meta.insert( + MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY.to_string(), + JsonValue::String(tool_description.clone()), + ); + } + if server == CODEX_APPS_MCP_SERVER_NAME + && (connector_id.is_some() || connector_name.is_some() || connector_description.is_some()) + { + meta.insert( + MCP_TOOL_APPROVAL_SOURCE_KEY.to_string(), + JsonValue::String(MCP_TOOL_APPROVAL_SOURCE_CONNECTOR.to_string()), + ); + if let Some(connector_id) = connector_id.as_deref() { meta.insert( - MCP_TOOL_APPROVAL_TOOL_TITLE_KEY.to_string(), - serde_json::Value::String(tool_title.clone()), + MCP_TOOL_APPROVAL_CONNECTOR_ID_KEY.to_string(), + JsonValue::String(connector_id.to_string()), ); } - if let Some(tool_description) = metadata.tool_description.as_ref() { + if let Some(connector_name) = connector_name.as_ref() { meta.insert( - MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY.to_string(), - serde_json::Value::String(tool_description.clone()), + MCP_TOOL_APPROVAL_CONNECTOR_NAME_KEY.to_string(), + JsonValue::String(connector_name.clone()), ); } - if server == CODEX_APPS_MCP_SERVER_NAME - && (metadata.connector_id.is_some() - || metadata.connector_name.is_some() - || metadata.connector_description.is_some()) - { + if let Some(connector_description) = connector_description.as_ref() { meta.insert( - MCP_TOOL_APPROVAL_SOURCE_KEY.to_string(), - serde_json::Value::String(MCP_TOOL_APPROVAL_SOURCE_CONNECTOR.to_string()), + MCP_TOOL_APPROVAL_CONNECTOR_DESCRIPTION_KEY.to_string(), + JsonValue::String(connector_description.clone()), ); - if let Some(connector_id) = metadata.connector_id.as_deref() { - meta.insert( - MCP_TOOL_APPROVAL_CONNECTOR_ID_KEY.to_string(), - serde_json::Value::String(connector_id.to_string()), - ); - } - if let Some(connector_name) = metadata.connector_name.as_ref() { - meta.insert( - MCP_TOOL_APPROVAL_CONNECTOR_NAME_KEY.to_string(), - serde_json::Value::String(connector_name.clone()), - ); - } - if let Some(connector_description) = metadata.connector_description.as_ref() { - meta.insert( - MCP_TOOL_APPROVAL_CONNECTOR_DESCRIPTION_KEY.to_string(), - serde_json::Value::String(connector_description.clone()), - ); - } } } if let Some(tool_params) = tool_params { @@ -1601,22 +1690,21 @@ fn build_mcp_tool_approval_elicitation_meta( tool_params_display, ); } - (!meta.is_empty()).then_some(serde_json::Value::Object(meta)) + + (!meta.is_empty()).then_some(JsonValue::Object(meta)) } fn build_mcp_tool_approval_display_params( - tool_params: Option<&serde_json::Value>, -) -> Option> { + tool_params: Option<&JsonValue>, +) -> Option> { let tool_params = tool_params?.as_object()?; let mut display_params = tool_params .iter() - .map( - |(name, value)| crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam { - name: name.clone(), - value: value.clone(), - display_name: name.clone(), - }, - ) + .map(|(name, value)| RenderedMcpToolApprovalParam { + name: name.clone(), + value: value.clone(), + display_name: name.clone(), + }) .collect::>(); display_params.sort_by(|left, right| left.name.cmp(&right.name)); Some(display_params) diff --git a/codex-rs/core/src/mcp_tool_call_tests.rs b/codex-rs/core/src/mcp_tool_call_tests.rs index 524138f017d4..f45ea6c38cbf 100644 --- a/codex-rs/core/src/mcp_tool_call_tests.rs +++ b/codex-rs/core/src/mcp_tool_call_tests.rs @@ -1,5 +1,7 @@ use super::*; use crate::config::ConfigBuilder; +use crate::guardian::GuardianApprovalRequest; +use crate::guardian::GuardianMcpAnnotations; use crate::session::tests::make_session_and_context; use crate::session::tests::make_session_and_context_with_rx; use crate::state::ActiveTurn; @@ -103,6 +105,24 @@ fn prompt_options( } } +fn approval_prompt_request( + server: &str, + tool_name: &str, + arguments: Option, + metadata: Option<&McpToolApprovalMetadata>, +) -> GuardianApprovalRequest { + build_mcp_tool_approval_request( + "call-1", + tool_name, + &McpInvocation { + server: server.to_string(), + tool: tool_name.to_string(), + arguments, + }, + metadata, + ) +} + fn install_mcp_permission_request_hook( session: &mut Session, turn_context: &TurnContext, @@ -280,15 +300,97 @@ fn prompt_mode_does_not_allow_persistent_remember() { #[test] fn approval_question_text_prepends_safety_reason() { + let request = approval_prompt_request( + "custom_server", + "run_action", + /*arguments*/ None, + /*metadata*/ None, + ); assert_eq!( - mcp_tool_approval_question_text( - "Allow this action?".to_string(), + mcp_tool_approval_prompt( + &request, + "q".to_string(), + prompt_options( + /*allow_session_remember*/ false, /*allow_persistent_approval*/ false, + ), Some("This tool may contact an external system."), - ), + ) + .expect("mcp approval prompt") + .question + .question, "Tool call needs your approval. Reason: This tool may contact an external system." ); } +#[test] +fn mcp_tool_approval_compat_response_uses_session_label_when_present() { + let request = approval_prompt_request( + "custom_server", + "dangerous_tool", + /*arguments*/ None, + /*metadata*/ None, + ); + let question = RequestUserInputQuestion { + id: "q-1".to_string(), + header: "Approve app tool call?".to_string(), + question: "Allow this app tool?".to_string(), + is_other: false, + is_secret: false, + options: Some(vec![RequestUserInputQuestionOption { + label: MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION.to_string(), + description: "Remember until session ends".to_string(), + }]), + }; + + let response = + mcp_tool_approval_compat_response(&request, &question, ReviewDecision::ApprovedForSession) + .expect("compat response"); + + assert_eq!( + response.answers.get("q-1"), + Some(&RequestUserInputAnswer { + answers: vec![MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION.to_string()], + }) + ); +} + +#[test] +fn mcp_tool_approval_compat_response_uses_synthetic_decline_for_abort() { + let request = approval_prompt_request( + "custom_server", + "dangerous_tool", + /*arguments*/ None, + /*metadata*/ None, + ); + let question = RequestUserInputQuestion { + id: "q-1".to_string(), + header: "Approve app tool call?".to_string(), + question: "Allow this app tool?".to_string(), + is_other: false, + is_secret: false, + options: None, + }; + + let response = mcp_tool_approval_compat_response(&request, &question, ReviewDecision::Abort) + .expect("compat response"); + + assert_eq!( + response.answers.get("q-1"), + Some(&RequestUserInputAnswer { + answers: vec![MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC.to_string()], + }) + ); +} + +#[test] +fn mcp_tool_approval_question_id_detection_round_trips() { + let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_call-1"); + + assert_eq!(question_id, "mcp_tool_call_approval_call-1"); + assert!(is_mcp_tool_approval_question_id(&question_id)); + assert!(!is_mcp_tool_approval_question_id("other_question")); +} + #[tokio::test] async fn mcp_tool_call_span_records_expected_fields() { let buffer: &'static std::sync::Mutex> = @@ -479,47 +581,43 @@ fn truncates_strings_on_char_boundaries() { #[tokio::test] async fn approval_elicitation_request_uses_message_override_and_preserves_tool_params_keys() { let (session, turn_context) = make_session_and_context().await; - let question = build_mcp_tool_approval_question( - "q".to_string(), + let metadata = approval_metadata( + Some("connector_947e0d954944416db111db556030eea6"), + Some("Calendar"), + Some("Manage events and schedules."), + Some("create_event"), + Some("Create a calendar event."), + ); + let approval_request = approval_prompt_request( CODEX_APPS_MCP_SERVER_NAME, "create_event", - Some("Calendar"), + Some(serde_json::json!({ + "title": "Roadmap review", + "start_time": "2026-05-01T10:00:00Z", + "attendees": ["ada@example.com"], + })), + Some(&metadata), + ); + let prompt = mcp_tool_approval_prompt( + &approval_request, + "q".to_string(), prompt_options( /*allow_session_remember*/ true, /*allow_persistent_approval*/ true, ), - Some("Allow Calendar to create an event?"), - ); + /*monitor_reason*/ None, + ) + .expect("mcp approval prompt"); let request = build_mcp_tool_approval_elicitation_request( &session, &turn_context, McpToolApprovalElicitationRequest { server: CODEX_APPS_MCP_SERVER_NAME, - metadata: Some(&approval_metadata( - Some("calendar"), - Some("Calendar"), - Some("Manage events and schedules."), - Some("Create Event"), - Some("Create a calendar event."), - )), - tool_params: Some(&serde_json::json!({ - "calendar_id": "primary", - "title": "Roadmap review", - })), - tool_params_display: Some(&[ - RenderedMcpToolApprovalParam { - name: "calendar_id".to_string(), - value: serde_json::json!("primary"), - display_name: "Calendar".to_string(), - }, - RenderedMcpToolApprovalParam { - name: "title".to_string(), - value: serde_json::json!("Roadmap review"), - display_name: "Title".to_string(), - }, - ]), - question, - message_override: Some("Allow Calendar to create an event?"), + approval_request: &approval_request, + tool_params: prompt.tool_params.as_ref(), + tool_params_display: prompt.tool_params_display.as_deref(), + question: prompt.question, + message_override: prompt.message_override.as_deref(), prompt_options: prompt_options( /*allow_session_remember*/ true, /*allow_persistent_approval*/ true, ), @@ -540,26 +638,32 @@ async fn approval_elicitation_request_uses_message_override_and_preserves_tool_p MCP_TOOL_APPROVAL_PERSIST_ALWAYS, ], MCP_TOOL_APPROVAL_SOURCE_KEY: MCP_TOOL_APPROVAL_SOURCE_CONNECTOR, - MCP_TOOL_APPROVAL_CONNECTOR_ID_KEY: "calendar", + MCP_TOOL_APPROVAL_CONNECTOR_ID_KEY: "connector_947e0d954944416db111db556030eea6", MCP_TOOL_APPROVAL_CONNECTOR_NAME_KEY: "Calendar", MCP_TOOL_APPROVAL_CONNECTOR_DESCRIPTION_KEY: "Manage events and schedules.", - MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: "Create Event", + MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: "create_event", MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: "Create a calendar event.", MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: { - "calendar_id": "primary", "title": "Roadmap review", + "start_time": "2026-05-01T10:00:00Z", + "attendees": ["ada@example.com"], }, MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY: [ - { - "name": "calendar_id", - "value": "primary", - "display_name": "Calendar", - }, { "name": "title", "value": "Roadmap review", "display_name": "Title", }, + { + "name": "start_time", + "value": "2026-05-01T10:00:00Z", + "display_name": "Start", + }, + { + "name": "attendees", + "value": ["ada@example.com"], + "display_name": "Attendees", + }, ], })), message: "Allow Calendar to create an event?".to_string(), @@ -576,16 +680,21 @@ async fn approval_elicitation_request_uses_message_override_and_preserves_tool_p #[test] fn custom_mcp_tool_question_mentions_server_name() { - let question = build_mcp_tool_approval_question( + let question = mcp_tool_approval_prompt( + &approval_prompt_request( + "custom_server", + "run_action", + /*arguments*/ None, + /*metadata*/ None, + ), "q".to_string(), - "custom_server", - "run_action", - /*connector_name*/ None, prompt_options( /*allow_session_remember*/ false, /*allow_persistent_approval*/ false, ), - /*question_override*/ None, - ); + /*monitor_reason*/ None, + ) + .expect("mcp approval prompt") + .question; assert_eq!(question.header, "Approve app tool call?"); assert_eq!( @@ -604,16 +713,21 @@ fn custom_mcp_tool_question_mentions_server_name() { #[test] fn codex_apps_tool_question_uses_fallback_app_label() { - let question = build_mcp_tool_approval_question( + let question = mcp_tool_approval_prompt( + &approval_prompt_request( + CODEX_APPS_MCP_SERVER_NAME, + "run_action", + /*arguments*/ None, + /*metadata*/ None, + ), "q".to_string(), - CODEX_APPS_MCP_SERVER_NAME, - "run_action", - /*connector_name*/ None, prompt_options( /*allow_session_remember*/ true, /*allow_persistent_approval*/ true, ), - /*question_override*/ None, - ); + /*monitor_reason*/ None, + ) + .expect("mcp approval prompt") + .question; assert_eq!( question.question, @@ -623,16 +737,28 @@ fn codex_apps_tool_question_uses_fallback_app_label() { #[test] fn trusted_codex_apps_tool_question_offers_always_allow() { - let question = build_mcp_tool_approval_question( - "q".to_string(), - CODEX_APPS_MCP_SERVER_NAME, - "run_action", + let metadata = approval_metadata( + Some("calendar"), Some("Calendar"), + /*connector_description*/ None, + /*tool_title*/ None, + /*tool_description*/ None, + ); + let question = mcp_tool_approval_prompt( + &approval_prompt_request( + CODEX_APPS_MCP_SERVER_NAME, + "run_action", + /*arguments*/ None, + Some(&metadata), + ), + "q".to_string(), prompt_options( /*allow_session_remember*/ true, /*allow_persistent_approval*/ true, ), - /*question_override*/ None, - ); + /*monitor_reason*/ None, + ) + .expect("mcp approval prompt") + .question; let options = question.options.expect("options"); assert!(options.iter().any(|option| { @@ -665,18 +791,30 @@ fn codex_apps_tool_question_without_elicitation_omits_always_allow() { 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", + let metadata = approval_metadata( + Some("calendar"), Some("Calendar"), + /*connector_description*/ None, + /*tool_title*/ None, + /*tool_description*/ None, + ); + let question = mcp_tool_approval_prompt( + &approval_prompt_request( + CODEX_APPS_MCP_SERVER_NAME, + "run_action", + /*arguments*/ None, + Some(&metadata), + ), + "q".to_string(), mcp_tool_approval_prompt_options( Some(&session_key), Some(&persistent_key), /*tool_call_mcp_elicitation_enabled*/ false, ), - /*question_override*/ None, - ); + /*monitor_reason*/ None, + ) + .expect("mcp approval prompt") + .question; assert_eq!( question @@ -695,16 +833,21 @@ fn codex_apps_tool_question_without_elicitation_omits_always_allow() { #[test] fn custom_mcp_tool_question_offers_session_remember_and_always_allow() { - let question = build_mcp_tool_approval_question( + let question = mcp_tool_approval_prompt( + &approval_prompt_request( + "custom_server", + "run_action", + /*arguments*/ None, + /*metadata*/ None, + ), "q".to_string(), - "custom_server", - "run_action", - /*connector_name*/ None, prompt_options( /*allow_session_remember*/ true, /*allow_persistent_approval*/ true, ), - /*question_override*/ None, - ); + /*monitor_reason*/ None, + ) + .expect("mcp approval prompt") + .question; assert_eq!( question @@ -1086,10 +1229,15 @@ fn accepted_elicitation_content_converts_to_request_user_input_response() { #[test] fn approval_elicitation_meta_marks_tool_approvals() { + let request = approval_prompt_request( + "custom_server", + "run_action", + /*arguments*/ None, + /*metadata*/ None, + ); assert_eq!( build_mcp_tool_approval_elicitation_meta( - "custom_server", - /*metadata*/ None, + &request, /*tool_params*/ None, /*tool_params_display*/ None, prompt_options( @@ -1104,16 +1252,22 @@ fn approval_elicitation_meta_marks_tool_approvals() { #[test] fn approval_elicitation_meta_merges_session_and_always_persist_for_custom_servers() { + let metadata = approval_metadata( + /*connector_id*/ None, + /*connector_name*/ None, + /*connector_description*/ None, + Some("Run Action"), + Some("Runs the selected action."), + ); + let request = approval_prompt_request( + "custom_server", + "run_action", + Some(serde_json::json!({"id": 1})), + Some(&metadata), + ); assert_eq!( build_mcp_tool_approval_elicitation_meta( - "custom_server", - Some(&approval_metadata( - /*connector_id*/ None, - /*connector_name*/ None, - /*connector_description*/ None, - Some("Run Action"), - Some("Runs the selected action."), - )), + &request, Some(&serde_json::json!({"id": 1})), /*tool_params_display*/ None, prompt_options( @@ -1145,8 +1299,9 @@ fn guardian_mcp_review_request_includes_invocation_metadata() { })), }; - let request = build_guardian_mcp_tool_review_request( + let request = build_mcp_tool_approval_request( "call-1", + "browser_navigate", &invocation, Some(&approval_metadata( Some("playwright"), @@ -1163,6 +1318,7 @@ fn guardian_mcp_review_request_includes_invocation_metadata() { id: "call-1".to_string(), server: CODEX_APPS_MCP_SERVER_NAME.to_string(), tool_name: "browser_navigate".to_string(), + hook_tool_name: "browser_navigate".to_string(), arguments: Some(serde_json::json!({ "url": "https://example.com", })), @@ -1195,7 +1351,8 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { openai_file_input_params: None, }; - let request = build_guardian_mcp_tool_review_request("call-1", &invocation, Some(&metadata)); + let request = + build_mcp_tool_approval_request("call-1", "dangerous_tool", &invocation, Some(&metadata)); assert_eq!( request, @@ -1203,6 +1360,7 @@ fn guardian_mcp_review_request_includes_annotations_when_present() { id: "call-1".to_string(), server: "custom_server".to_string(), tool_name: "dangerous_tool".to_string(), + hook_tool_name: "dangerous_tool".to_string(), arguments: None, connector_id: None, connector_name: None, @@ -1316,16 +1474,24 @@ async fn guardian_review_decision_maps_to_mcp_tool_decision() { #[test] fn approval_elicitation_meta_includes_connector_source_for_codex_apps() { + let metadata = approval_metadata( + Some("calendar"), + Some("Calendar"), + Some("Manage events and schedules."), + Some("Run Action"), + Some("Runs the selected action."), + ); + let request = approval_prompt_request( + CODEX_APPS_MCP_SERVER_NAME, + "run_action", + Some(serde_json::json!({ + "calendar_id": "primary", + })), + Some(&metadata), + ); assert_eq!( build_mcp_tool_approval_elicitation_meta( - CODEX_APPS_MCP_SERVER_NAME, - Some(&approval_metadata( - Some("calendar"), - Some("Calendar"), - Some("Manage events and schedules."), - Some("Run Action"), - Some("Runs the selected action."), - )), + &request, Some(&serde_json::json!({ "calendar_id": "primary", })), @@ -1351,16 +1517,24 @@ fn approval_elicitation_meta_includes_connector_source_for_codex_apps() { #[test] fn approval_elicitation_meta_merges_session_and_always_persist_with_connector_source() { + let metadata = approval_metadata( + Some("calendar"), + Some("Calendar"), + Some("Manage events and schedules."), + Some("Run Action"), + Some("Runs the selected action."), + ); + let request = approval_prompt_request( + CODEX_APPS_MCP_SERVER_NAME, + "run_action", + Some(serde_json::json!({ + "calendar_id": "primary", + })), + Some(&metadata), + ); assert_eq!( build_mcp_tool_approval_elicitation_meta( - CODEX_APPS_MCP_SERVER_NAME, - Some(&approval_metadata( - Some("calendar"), - Some("Calendar"), - Some("Manage events and schedules."), - Some("Run Action"), - Some("Runs the selected action."), - )), + &request, Some(&serde_json::json!({ "calendar_id": "primary", })), diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index c18976fde1a4..57f5dab799b0 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -32,6 +32,7 @@ use crate::context::PersonalitySpecInstructions; use crate::default_skill_metadata_budget; use crate::environment_selection::ResolvedTurnEnvironments; use crate::exec_policy::ExecPolicyManager; +use crate::guardian::GuardianApprovalRequest; use crate::installation_id::resolve_installation_id; use crate::parse_turn_item; use crate::path_utils::normalize_for_native_workdir; @@ -99,7 +100,6 @@ use codex_protocol::models::format_allow_prefixes; use codex_protocol::openai_models::ModelInfo; use codex_protocol::permissions::FileSystemSandboxPolicy; use codex_protocol::permissions::NetworkSandboxPolicy; -use codex_protocol::protocol::FileChange; use codex_protocol::protocol::HasLegacyEvent; use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::ItemCompletedEvent; @@ -116,7 +116,6 @@ use codex_protocol::protocol::W3cTraceContext; use codex_protocol::request_permissions::PermissionGrantScope; use codex_protocol::request_permissions::RequestPermissionProfile; use codex_protocol::request_permissions::RequestPermissionsArgs; -use codex_protocol::request_permissions::RequestPermissionsEvent; use codex_protocol::request_permissions::RequestPermissionsResponse; use codex_protocol::request_user_input::RequestUserInputArgs; use codex_protocol::request_user_input::RequestUserInputResponse; @@ -126,7 +125,6 @@ use codex_rollout_trace::AgentResultTracePayload; use codex_rollout_trace::ThreadStartedTraceMetadata; use codex_rollout_trace::ThreadTraceContext; use codex_sandboxing::policy_transforms::intersect_permission_profiles; -use codex_shell_command::parse_command::parse_command; use codex_terminal_detection::user_agent; use codex_thread_store::CreateThreadParams; use codex_thread_store::LiveThread; @@ -1844,26 +1842,56 @@ impl Session { /// be used to derive the available decisions via /// [ExecApprovalRequestEvent::default_available_decisions]. #[allow(clippy::too_many_arguments)] - #[expect( - clippy::await_holding_invalid_type, - reason = "active turn checks and turn state updates must remain atomic" - )] - pub async fn request_command_approval( + pub async fn request_command_approval_for_request( &self, turn_context: &TurnContext, - call_id: String, + approval_request: GuardianApprovalRequest, approval_id: Option, - command: Vec, - cwd: AbsolutePathBuf, reason: Option, network_approval_context: Option, proposed_execpolicy_amendment: Option, - additional_permissions: Option, available_decisions: Option>, + fallback_cwd: Option, + ) -> ReviewDecision { + let proposed_network_policy_amendments = network_approval_context.as_ref().map(|context| { + vec![ + NetworkPolicyAmendment { + host: context.host.clone(), + action: NetworkPolicyRuleAction::Allow, + }, + NetworkPolicyAmendment { + host: context.host.clone(), + action: NetworkPolicyRuleAction::Deny, + }, + ] + }); + let Some(event) = approval_request.exec_approval_event( + turn_context.sub_id.clone(), + approval_id, + reason, + network_approval_context, + proposed_execpolicy_amendment, + proposed_network_policy_amendments, + available_decisions, + fallback_cwd, + ) else { + unreachable!("request_command_approval_for_request requires command-like approvals"); + }; + self.request_exec_approval_event(turn_context, event).await + } + + #[expect( + clippy::await_holding_invalid_type, + reason = "active turn checks and turn state updates must remain atomic" + )] + async fn request_exec_approval_event( + &self, + turn_context: &TurnContext, + event: ExecApprovalRequestEvent, ) -> ReviewDecision { // command-level approvals use `call_id`. // `approval_id` is only present for subcommand callbacks (execve intercept) - let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); + let effective_approval_id = event.effective_approval_id(); // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); let prev_entry = { @@ -1880,60 +1908,55 @@ impl Session { warn!("Overwriting existing pending approval for call_id: {effective_approval_id}"); } - let parsed_cmd = parse_command(&command); - let proposed_network_policy_amendments = network_approval_context.as_ref().map(|context| { - vec![ - NetworkPolicyAmendment { - host: context.host.clone(), - action: NetworkPolicyRuleAction::Allow, - }, - NetworkPolicyAmendment { - host: context.host.clone(), - action: NetworkPolicyRuleAction::Deny, - }, - ] - }); - let available_decisions = available_decisions.unwrap_or_else(|| { + let available_decisions = event.available_decisions.clone().unwrap_or_else(|| { ExecApprovalRequestEvent::default_available_decisions( - network_approval_context.as_ref(), - proposed_execpolicy_amendment.as_ref(), - proposed_network_policy_amendments.as_deref(), - additional_permissions.as_ref(), + event.network_approval_context.as_ref(), + event.proposed_execpolicy_amendment.as_ref(), + event.proposed_network_policy_amendments.as_deref(), + event.additional_permissions.as_ref(), ) }); - let event = EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { - call_id, - approval_id, - turn_id: turn_context.sub_id.clone(), - command, - cwd, - reason, - network_approval_context, - proposed_execpolicy_amendment, - proposed_network_policy_amendments, - additional_permissions, - available_decisions: Some(available_decisions), - parsed_cmd, - }); - self.send_event(turn_context, event).await; + self.send_event( + turn_context, + EventMsg::ExecApprovalRequest(ExecApprovalRequestEvent { + available_decisions: Some(available_decisions), + ..event + }), + ) + .await; rx_approve.await.unwrap_or(ReviewDecision::Abort) } + pub async fn request_patch_approval_for_request( + &self, + turn_context: &TurnContext, + approval_request: GuardianApprovalRequest, + reason: Option, + grant_root: Option, + ) -> oneshot::Receiver { + let Some(event) = approval_request.apply_patch_approval_event( + turn_context.sub_id.clone(), + reason, + grant_root, + ) else { + unreachable!("request_patch_approval_for_request requires apply_patch approvals"); + }; + self.request_apply_patch_approval_event(turn_context, event) + .await + } + #[expect( clippy::await_holding_invalid_type, reason = "active turn checks and turn state updates must remain atomic" )] - pub async fn request_patch_approval( + async fn request_apply_patch_approval_event( &self, turn_context: &TurnContext, - call_id: String, - changes: HashMap, - reason: Option, - grant_root: Option, + event: ApplyPatchApprovalRequestEvent, ) -> oneshot::Receiver { // Add the tx_approve callback to the map before sending the request. let (tx_approve, rx_approve) = oneshot::channel(); - let approval_id = call_id.clone(); + let approval_id = event.call_id.clone(); let prev_entry = { let mut active = self.active_turn.lock().await; match active.as_mut() { @@ -1948,14 +1971,8 @@ impl Session { warn!("Overwriting existing pending approval for call_id: {approval_id}"); } - let event = EventMsg::ApplyPatchApprovalRequest(ApplyPatchApprovalRequestEvent { - call_id, - turn_id: turn_context.sub_id.clone(), - changes, - reason, - grant_root, - }); - self.send_event(turn_context, event).await; + self.send_event(turn_context, EventMsg::ApplyPatchApprovalRequest(event)) + .await; rx_approve } @@ -2012,6 +2029,13 @@ impl Session { } let requested_permissions = args.permissions; + let approval_request = GuardianApprovalRequest::RequestPermissions { + id: call_id.clone(), + turn_id: turn_context.sub_id.clone(), + reason: args.reason.clone(), + permissions: requested_permissions.clone(), + cwd: cwd.clone(), + }; if crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) { let originating_turn_state = { @@ -2021,17 +2045,11 @@ impl Session { let review_id = crate::guardian::new_guardian_review_id(); let session = Arc::clone(self); let turn = Arc::clone(turn_context); - let request = crate::guardian::GuardianApprovalRequest::RequestPermissions { - id: call_id, - turn_id: turn_context.sub_id.clone(), - reason: args.reason, - permissions: requested_permissions.clone(), - }; let review_rx = crate::guardian::spawn_approval_request_review( session, turn, review_id, - request, + approval_request.clone(), /*retry_reason*/ None, codex_analytics::GuardianApprovalRequestSource::MainTurn, cancellation_token.clone(), @@ -2111,14 +2129,11 @@ impl Session { warn!("Overwriting existing pending request_permissions for call_id: {call_id}"); } - let event = EventMsg::RequestPermissions(RequestPermissionsEvent { - call_id: call_id.clone(), - turn_id: turn_context.sub_id.clone(), - reason: args.reason, - permissions: requested_permissions, - cwd: Some(cwd), - }); - self.send_event(turn_context.as_ref(), event).await; + let Some(event) = approval_request.request_permissions_event() else { + unreachable!("request_permissions event projection must exist"); + }; + self.send_event(turn_context.as_ref(), EventMsg::RequestPermissions(event)) + .await; tokio::select! { biased; _ = cancellation_token.cancelled() => { diff --git a/codex-rs/core/src/tools/network_approval.rs b/codex-rs/core/src/tools/network_approval.rs index 14af2c9c5f3c..31c6ecdb9558 100644 --- a/codex-rs/core/src/tools/network_approval.rs +++ b/codex-rs/core/src/tools/network_approval.rs @@ -8,7 +8,6 @@ use crate::guardian::routes_approval_to_guardian; use crate::hook_runtime::run_permission_request_hooks; use crate::network_policy_decision::denied_network_policy_message; use crate::session::session::Session; -use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::ToolError; use codex_hooks::PermissionRequestDecision; use codex_network_proxy::BlockedRequest; @@ -460,17 +459,30 @@ impl NetworkApprovalService { protocol, }; let guardian_approval_id = Self::approval_id_for_key(&key); - let prompt_command = vec!["network-access".to_string(), target.clone()]; - let command = owner_call - .as_ref() - .map_or_else(|| prompt_command.join(" "), |call| call.command.clone()); - if let Some(permission_request_decision) = run_permission_request_hooks( - &session, - &turn_context, - &guardian_approval_id, - PermissionRequestPayload::bash(command, Some(format!("network-access {target}"))), - ) - .await + let command = owner_call.as_ref().map_or_else( + || format!("network-access {target}"), + |call| call.command.clone(), + ); + let approval_request = GuardianApprovalRequest::NetworkAccess { + id: guardian_approval_id.clone(), + turn_id: owner_call + .as_ref() + .map_or_else(|| turn_context.sub_id.clone(), |call| call.turn_id.clone()), + target: target.clone(), + hook_command: command, + host: request.host.clone(), + protocol, + port: key.port, + trigger: owner_call.as_ref().map(|call| call.trigger.clone()), + }; + if let Some(permission_request_decision) = + run_permission_request_hooks(&session, &turn_context, &guardian_approval_id, { + let Some(payload) = approval_request.permission_request_payload() else { + unreachable!("network approvals always project a permission request payload"); + }; + payload + }) + .await { match permission_request_decision { PermissionRequestDecision::Allow => { @@ -503,34 +515,22 @@ impl NetworkApprovalService { &session, &turn_context, review_id, - GuardianApprovalRequest::NetworkAccess { - id: guardian_approval_id.clone(), - turn_id: owner_call - .as_ref() - .map_or_else(|| turn_context.sub_id.clone(), |call| call.turn_id.clone()), - target, - host: request.host, - protocol, - port: key.port, - trigger: owner_call.as_ref().map(|call| call.trigger.clone()), - }, + approval_request, Some(policy_denial_message.clone()), ) .await } else { let available_decisions = None; session - .request_command_approval( + .request_command_approval_for_request( turn_context.as_ref(), - guardian_approval_id, + approval_request, /*approval_id*/ None, - prompt_command, - turn_context.cwd.clone(), Some(prompt_reason), - Some(network_approval_context.clone()), + /*network_approval_context*/ None, /*proposed_execpolicy_amendment*/ None, - /*additional_permissions*/ None, available_decisions, + Some(turn_context.cwd.clone()), ) .await }; diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs index dcb42c36c6e5..54227006dc6e 100644 --- a/codex-rs/core/src/tools/orchestrator.rs +++ b/codex-rs/core/src/tools/orchestrator.rs @@ -394,8 +394,11 @@ impl ToolOrchestrator { where T: ToolRuntime, { + let approval_request = tool.approval_request(req, &approval_ctx); if evaluate_permission_request_hooks - && let Some(permission_request) = tool.permission_request_payload(req) + && let Some(permission_request) = approval_request + .as_ref() + .and_then(crate::guardian::GuardianApprovalRequest::permission_request_payload) { match run_permission_request_hooks( approval_ctx.session, diff --git a/codex-rs/core/src/tools/runtimes/apply_patch.rs b/codex-rs/core/src/tools/runtimes/apply_patch.rs index e720243f2bfe..35b5cfde45ca 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch.rs @@ -6,11 +6,9 @@ use crate::exec::is_likely_sandbox_denied; use crate::guardian::GuardianApprovalRequest; use crate::guardian::review_approval_request; -use crate::tools::hook_names::HookToolName; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; -use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::Sandboxable; use crate::tools::sandboxing::ToolCtx; @@ -53,14 +51,12 @@ impl ApplyPatchRuntime { Self } - fn build_guardian_review_request( - req: &ApplyPatchRequest, - call_id: &str, - ) -> GuardianApprovalRequest { + fn build_approval_request(req: &ApplyPatchRequest, call_id: &str) -> GuardianApprovalRequest { GuardianApprovalRequest::ApplyPatch { id: call_id.to_string(), cwd: req.action.cwd.clone(), files: req.file_paths.clone(), + changes: req.changes.clone(), patch: req.action.patch.clone(), } } @@ -108,26 +104,29 @@ impl Approvable for ApplyPatchRuntime { ) -> BoxFuture<'a, ReviewDecision> { let session = ctx.session; let turn = ctx.turn; - let call_id = ctx.call_id.to_string(); let retry_reason = ctx.retry_reason.clone(); let approval_keys = self.approval_keys(req); - let changes = req.changes.clone(); let guardian_review_id = ctx.guardian_review_id.clone(); + let approval_request = Self::build_approval_request(req, ctx.call_id); Box::pin(async move { if let Some(review_id) = guardian_review_id { - let action = ApplyPatchRuntime::build_guardian_review_request(req, ctx.call_id); - return review_approval_request(session, turn, review_id, action, retry_reason) - .await; + return review_approval_request( + session, + turn, + review_id, + approval_request.clone(), + retry_reason, + ) + .await; } if req.permissions_preapproved && retry_reason.is_none() { return ReviewDecision::Approved; } if let Some(reason) = retry_reason { let rx_approve = session - .request_patch_approval( + .request_patch_approval_for_request( turn, - call_id, - changes.clone(), + approval_request.clone(), Some(reason), /*grant_root*/ None, ) @@ -141,8 +140,11 @@ impl Approvable for ApplyPatchRuntime { approval_keys, || async move { let rx_approve = session - .request_patch_approval( - turn, call_id, changes, /*reason*/ None, /*grant_root*/ None, + .request_patch_approval_for_request( + turn, + approval_request.clone(), + /*reason*/ None, + /*grant_root*/ None, ) .await; rx_approve.await.unwrap_or_default() @@ -173,14 +175,12 @@ impl Approvable for ApplyPatchRuntime { Some(req.exec_approval_requirement.clone()) } - fn permission_request_payload( + fn approval_request( &self, req: &ApplyPatchRequest, - ) -> Option { - Some(PermissionRequestPayload { - tool_name: HookToolName::apply_patch(), - tool_input: serde_json::json!({ "command": req.action.patch }), - }) + ctx: &ApprovalCtx<'_>, + ) -> Option { + Some(Self::build_approval_request(req, ctx.call_id)) } } diff --git a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs index 173fa3e2a0bb..fa908c7176cd 100644 --- a/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs +++ b/codex-rs/core/src/tools/runtimes/apply_patch_tests.rs @@ -1,4 +1,5 @@ use super::*; +use crate::guardian::GuardianApprovalRequest; use crate::tools::sandboxing::SandboxAttempt; use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::AdditionalPermissionProfile; @@ -65,7 +66,7 @@ fn guardian_review_request_includes_patch_context() { permissions_preapproved: false, }; - let guardian_request = ApplyPatchRuntime::build_guardian_review_request(&request, "call-1"); + let guardian_request = ApplyPatchRuntime::build_approval_request(&request, "call-1"); assert_eq!( guardian_request, @@ -73,6 +74,7 @@ fn guardian_review_request_includes_patch_context() { id: "call-1".to_string(), cwd: expected_cwd, files: request.file_paths, + changes: request.changes, patch: expected_patch, } ); @@ -80,7 +82,6 @@ fn guardian_review_request_includes_patch_context() { #[test] fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() { - let runtime = ApplyPatchRuntime::new(); let path = std::env::temp_dir() .join("apply-patch-permission-request-payload.txt") .abs(); @@ -98,8 +99,8 @@ fn permission_request_payload_uses_apply_patch_hook_name_and_aliases() { permissions_preapproved: false, }; - let payload = runtime - .permission_request_payload(&req) + let payload = ApplyPatchRuntime::build_approval_request(&req, "call-1") + .permission_request_payload() .expect("permission request payload"); assert_eq!(payload.tool_name.name(), "apply_patch"); diff --git a/codex-rs/core/src/tools/runtimes/shell.rs b/codex-rs/core/src/tools/runtimes/shell.rs index 7f17285db9d9..5d77c64d1f1f 100644 --- a/codex-rs/core/src/tools/runtimes/shell.rs +++ b/codex-rs/core/src/tools/runtimes/shell.rs @@ -25,7 +25,6 @@ use crate::tools::runtimes::maybe_wrap_shell_lc_with_snapshot; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; -use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; use crate::tools::sandboxing::Sandboxable; @@ -120,6 +119,18 @@ impl ShellRuntime { tx_event: ctx.session.get_tx_event(), }) } + + fn build_approval_request(req: &ShellRequest, call_id: String) -> GuardianApprovalRequest { + GuardianApprovalRequest::Shell { + id: call_id, + command: req.command.clone(), + hook_command: req.hook_command.clone(), + cwd: req.cwd.clone(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + } + } } impl Sandboxable for ShellRuntime { @@ -149,13 +160,11 @@ impl Approvable for ShellRuntime { ctx: ApprovalCtx<'a>, ) -> BoxFuture<'a, ReviewDecision> { let keys = self.approval_keys(req); - let command = req.command.clone(); - let cwd = req.cwd.clone(); let retry_reason = ctx.retry_reason.clone(); let reason = retry_reason.clone().or_else(|| req.justification.clone()); let session = ctx.session; let turn = ctx.turn; - let call_id = ctx.call_id.to_string(); + let approval_request = Self::build_approval_request(req, ctx.call_id.to_string()); let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { if let Some(review_id) = guardian_review_id { @@ -163,14 +172,7 @@ impl Approvable for ShellRuntime { session, turn, review_id, - GuardianApprovalRequest::Shell { - id: call_id, - command, - cwd: cwd.clone(), - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - }, + approval_request.clone(), retry_reason, ) .await; @@ -178,19 +180,17 @@ impl Approvable for ShellRuntime { with_cached_approval(&session.services, "shell", keys, move || async move { let available_decisions = None; session - .request_command_approval( + .request_command_approval_for_request( turn, - call_id, + approval_request.clone(), /*approval_id*/ None, - command, - cwd, reason, ctx.network_approval_context.clone(), req.exec_approval_requirement .proposed_execpolicy_amendment() .cloned(), - req.additional_permissions.clone(), available_decisions, + /*fallback_cwd*/ None, ) .await }) @@ -202,11 +202,12 @@ impl Approvable for ShellRuntime { Some(req.exec_approval_requirement.clone()) } - fn permission_request_payload(&self, req: &ShellRequest) -> Option { - Some(PermissionRequestPayload::bash( - req.hook_command.clone(), - req.justification.clone(), - )) + fn approval_request( + &self, + req: &ShellRequest, + ctx: &ApprovalCtx<'_>, + ) -> Option { + Some(Self::build_approval_request(req, ctx.call_id.to_string())) } fn sandbox_mode_for_first_attempt(&self, req: &ShellRequest) -> SandboxOverride { diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index fef8db5ca9e5..f1ae85bec501 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -16,7 +16,6 @@ use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; use crate::tools::runtimes::build_sandbox_command; use crate::tools::runtimes::exec_env_for_sandbox_permissions; -use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; use crate::tools::sandboxing::ToolError; @@ -396,7 +395,6 @@ impl CoreShellActionProvider { stopwatch: &Stopwatch, additional_permissions: Option, ) -> anyhow::Result { - let command = join_program_and_argv(program, argv); let workdir = workdir.clone(); let session = self.session.clone(); let turn = self.turn.clone(); @@ -407,17 +405,23 @@ impl CoreShellActionProvider { Ok(stopwatch .pause_for(async move { // 1) Run PermissionRequest hooks - let permission_request = PermissionRequestPayload::bash( - codex_shell_command::parse_command::shlex_join(&command), - /*description*/ None, - ); let effective_approval_id = approval_id.clone().unwrap_or_else(|| call_id.clone()); - match run_permission_request_hooks( - &session, - &turn, - &effective_approval_id, - permission_request, - ) + let approval_request = GuardianApprovalRequest::Execve { + id: call_id.clone(), + source, + program: program.to_string_lossy().into_owned(), + argv: argv.to_vec(), + cwd: workdir.clone(), + additional_permissions: additional_permissions.clone(), + }; + match run_permission_request_hooks(&session, &turn, &effective_approval_id, { + let Some(payload) = approval_request.permission_request_payload() else { + unreachable!( + "execve approvals always project a permission request payload" + ); + }; + payload + }) .await { Some(PermissionRequestDecision::Allow) => { @@ -443,14 +447,7 @@ impl CoreShellActionProvider { &session, &turn, review_id.clone(), - GuardianApprovalRequest::Execve { - id: call_id.clone(), - source, - program: program.to_string_lossy().into_owned(), - argv: argv.to_vec(), - cwd: workdir.clone(), - additional_permissions, - }, + approval_request, /*retry_reason*/ None, ) .await; @@ -463,17 +460,15 @@ impl CoreShellActionProvider { // 3) Fall back to regular user prompt let decision = session - .request_command_approval( + .request_command_approval_for_request( &turn, - call_id, + approval_request, approval_id, - command, - workdir.clone(), /*reason*/ None, /*network_approval_context*/ None, /*proposed_execpolicy_amendment*/ None, - additional_permissions, Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]), + /*fallback_cwd*/ None, ) .await; PromptDecision { diff --git a/codex-rs/core/src/tools/runtimes/unified_exec.rs b/codex-rs/core/src/tools/runtimes/unified_exec.rs index 520616823044..55c64fd6d006 100644 --- a/codex-rs/core/src/tools/runtimes/unified_exec.rs +++ b/codex-rs/core/src/tools/runtimes/unified_exec.rs @@ -23,7 +23,6 @@ use crate::tools::runtimes::shell::zsh_fork_backend; use crate::tools::sandboxing::Approvable; use crate::tools::sandboxing::ApprovalCtx; use crate::tools::sandboxing::ExecApprovalRequirement; -use crate::tools::sandboxing::PermissionRequestPayload; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::SandboxOverride; use crate::tools::sandboxing::Sandboxable; @@ -110,6 +109,22 @@ impl<'a> UnifiedExecRuntime<'a> { shell_mode, } } + + fn build_approval_request( + req: &UnifiedExecRequest, + call_id: String, + ) -> GuardianApprovalRequest { + GuardianApprovalRequest::ExecCommand { + id: call_id, + command: req.command.clone(), + hook_command: req.hook_command.clone(), + cwd: req.cwd.clone(), + sandbox_permissions: req.sandbox_permissions, + additional_permissions: req.additional_permissions.clone(), + justification: req.justification.clone(), + tty: req.tty, + } + } } impl Sandboxable for UnifiedExecRuntime<'_> { @@ -143,11 +158,9 @@ impl Approvable for UnifiedExecRuntime<'_> { let keys = self.approval_keys(req); let session = ctx.session; let turn = ctx.turn; - let call_id = ctx.call_id.to_string(); - let command = req.command.clone(); - let cwd = req.cwd.clone(); let retry_reason = ctx.retry_reason.clone(); let reason = retry_reason.clone().or_else(|| req.justification.clone()); + let approval_request = Self::build_approval_request(req, ctx.call_id.to_string()); let guardian_review_id = ctx.guardian_review_id.clone(); Box::pin(async move { if let Some(review_id) = guardian_review_id { @@ -155,15 +168,7 @@ impl Approvable for UnifiedExecRuntime<'_> { session, turn, review_id, - GuardianApprovalRequest::ExecCommand { - id: call_id, - command, - cwd: cwd.clone(), - sandbox_permissions: req.sandbox_permissions, - additional_permissions: req.additional_permissions.clone(), - justification: req.justification.clone(), - tty: req.tty, - }, + approval_request.clone(), retry_reason, ) .await; @@ -171,19 +176,17 @@ impl Approvable for UnifiedExecRuntime<'_> { with_cached_approval(&session.services, "unified_exec", keys, || async move { let available_decisions = None; session - .request_command_approval( + .request_command_approval_for_request( turn, - call_id, + approval_request.clone(), /*approval_id*/ None, - command, - cwd.clone(), reason, ctx.network_approval_context.clone(), req.exec_approval_requirement .proposed_execpolicy_amendment() .cloned(), - req.additional_permissions.clone(), available_decisions, + /*fallback_cwd*/ None, ) .await }) @@ -198,14 +201,12 @@ impl Approvable for UnifiedExecRuntime<'_> { Some(req.exec_approval_requirement.clone()) } - fn permission_request_payload( + fn approval_request( &self, req: &UnifiedExecRequest, - ) -> Option { - Some(PermissionRequestPayload::bash( - req.hook_command.clone(), - req.justification.clone(), - )) + ctx: &ApprovalCtx<'_>, + ) -> Option { + Some(Self::build_approval_request(req, ctx.call_id.to_string())) } fn sandbox_mode_for_first_attempt(&self, req: &UnifiedExecRequest) -> SandboxOverride { diff --git a/codex-rs/core/src/tools/sandboxing.rs b/codex-rs/core/src/tools/sandboxing.rs index 122cd00fad6f..2ef5c8f2b738 100644 --- a/codex-rs/core/src/tools/sandboxing.rs +++ b/codex-rs/core/src/tools/sandboxing.rs @@ -4,6 +4,7 @@ //! `ApprovalCtx`, `Approvable`) together with the sandbox orchestration traits //! and helpers (`Sandboxable`, `ToolRuntime`, `SandboxAttempt`, etc.). +use crate::guardian::GuardianApprovalRequest; use crate::sandboxing::ExecOptions; use crate::sandboxing::SandboxPermissions; use crate::session::session::Session; @@ -309,9 +310,13 @@ pub(crate) trait Approvable { None } - /// Return hook input for approval-time policy hooks when this runtime wants - /// hook evaluation to run before guardian or user approval. - fn permission_request_payload(&self, _req: &Req) -> Option { + /// Return the canonical approval action for this request when the runtime + /// participates in approval hooks and/or guardian review. + fn approval_request( + &self, + _req: &Req, + _ctx: &ApprovalCtx<'_>, + ) -> Option { None }