centralize approval prompts

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::<Vec<_>>()
+        .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::<Vec<_>>();
+            .collect::<Vec<_>>(),
+        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::<Vec<_>>()
-            .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(

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()],
                 },