refactor: adding allow_prefix into ApprovedAllowPrefix

zhao-oai · zhao-oai · commit d012bf51e43c · 2025-11-21T12:15:09.000-05:00
diff --git a/codex-rs/app-server/src/bespoke_event_handling.rs b/codex-rs/app-server/src/bespoke_event_handling.rs
@@ -611,7 +611,6 @@ async fn on_exec_approval_response(
         .submit(Op::ExecApproval {
             id: event_id,
             decision: response.decision,
-            allow_prefix: None,
         })
         .await
     {
@@ -785,7 +784,6 @@ async fn on_command_execution_request_approval_response(
         .submit(Op::ExecApproval {
             id: event_id,
             decision,
-            allow_prefix: None,
         })
         .await
     {
diff --git a/codex-rs/core/src/apply_patch.rs b/codex-rs/core/src/apply_patch.rs
@@ -71,7 +71,7 @@ pub(crate) async fn apply_patch(
                 .await;
             match rx_approve.await.unwrap_or_default() {
                 ReviewDecision::Approved
-                | ReviewDecision::ApprovedAllowPrefix
+                | ReviewDecision::ApprovedAllowPrefix { .. }
                 | ReviewDecision::ApprovedForSession => {
                     InternalApplyPatchInvocation::DelegateToExec(ApplyPatchExec {
                         action,
diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs
@@ -1424,12 +1424,8 @@ async fn submission_loop(sess: Arc<Session>, config: Arc<Config>, rx_sub: Receiv
                 handlers::user_input_or_turn(&sess, sub.id.clone(), sub.op, &mut previous_context)
                     .await;
             }
-            Op::ExecApproval {
-                id,
-                decision,
-                allow_prefix,
-            } => {
-                handlers::exec_approval(&sess, id, decision, allow_prefix).await;
+            Op::ExecApproval { id, decision } => {
+                handlers::exec_approval(&sess, id, decision).await;
             }
             Op::PatchApproval { id, decision } => {
                 handlers::patch_approval(&sess, id, decision).await;
@@ -1584,15 +1580,9 @@ mod handlers {
         *previous_context = Some(turn_context);
     }
 
-    pub async fn exec_approval(
-        sess: &Arc<Session>,
-        id: String,
-        decision: ReviewDecision,
-        allow_prefix: Option<Vec<String>>,
-    ) {
-        if let Some(prefix) = allow_prefix
-            && matches!(decision, ReviewDecision::ApprovedAllowPrefix)
-            && let Err(err) = sess.persist_command_allow_prefix(&prefix).await
+    pub async fn exec_approval(sess: &Arc<Session>, id: String, decision: ReviewDecision) {
+        if let ReviewDecision::ApprovedAllowPrefix { allow_prefix } = &decision
+            && let Err(err) = sess.persist_command_allow_prefix(allow_prefix).await
         {
             let message = format!("Failed to update execpolicy allow list: {err}");
             tracing::warn!("{message}");
diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs
@@ -245,13 +245,7 @@ async fn handle_exec_approval(
     )
     .await;
 
-    let _ = codex
-        .submit(Op::ExecApproval {
-            id,
-            decision,
-            allow_prefix: None,
-        })
-        .await;
+    let _ = codex.submit(Op::ExecApproval { id, decision }).await;
 }
 
 /// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying.
diff --git a/codex-rs/core/src/tools/orchestrator.rs b/codex-rs/core/src/tools/orchestrator.rs
@@ -87,14 +87,14 @@ impl ToolOrchestrator {
                 };
                 let decision = tool.start_approval_async(req, approval_ctx).await;
 
-                otel.tool_decision(otel_tn, otel_ci, decision, otel_user.clone());
+                otel.tool_decision(otel_tn, otel_ci, decision.clone(), otel_user.clone());
 
                 match decision {
                     ReviewDecision::Denied | ReviewDecision::Abort => {
                         return Err(ToolError::Rejected("rejected by user".to_string()));
                     }
                     ReviewDecision::Approved
-                    | ReviewDecision::ApprovedAllowPrefix
+                    | ReviewDecision::ApprovedAllowPrefix { .. }
                     | ReviewDecision::ApprovedForSession => {}
                 }
                 already_approved = true;
@@ -169,14 +169,14 @@ impl ToolOrchestrator {
                     };
 
                     let decision = tool.start_approval_async(req, approval_ctx).await;
-                    otel.tool_decision(otel_tn, otel_ci, decision, otel_user);
+                    otel.tool_decision(otel_tn, otel_ci, decision.clone(), otel_user);
 
                     match decision {
                         ReviewDecision::Denied | ReviewDecision::Abort => {
                             return Err(ToolError::Rejected("rejected by user".to_string()));
                         }
                         ReviewDecision::Approved
-                        | ReviewDecision::ApprovedAllowPrefix
+                        | ReviewDecision::ApprovedAllowPrefix { .. }
                         | ReviewDecision::ApprovedForSession => {}
                     }
                 }
diff --git a/codex-rs/core/tests/suite/approvals.rs b/codex-rs/core/tests/suite/approvals.rs
@@ -1523,8 +1523,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
             test.codex
                 .submit(Op::ExecApproval {
                     id: "0".into(),
-                    decision: *decision,
-                    allow_prefix: None,
+                    decision: decision.clone(),
                 })
                 .await?;
             wait_for_completion(&test).await;
@@ -1545,7 +1544,7 @@ async fn run_scenario(scenario: &ScenarioSpec) -> Result<()> {
             test.codex
                 .submit(Op::PatchApproval {
                     id: "0".into(),
-                    decision: *decision,
+                    decision: decision.clone(),
                 })
                 .await?;
             wait_for_completion(&test).await;
diff --git a/codex-rs/core/tests/suite/codex_delegate.rs b/codex-rs/core/tests/suite/codex_delegate.rs
@@ -93,7 +93,6 @@ async fn codex_delegate_forwards_exec_approval_and_proceeds_on_approval() {
         .submit(Op::ExecApproval {
             id: "0".into(),
             decision: ReviewDecision::Approved,
-            allow_prefix: None,
         })
         .await
         .expect("submit exec approval");
diff --git a/codex-rs/core/tests/suite/otel.rs b/codex-rs/core/tests/suite/otel.rs
@@ -843,7 +843,6 @@ async fn handle_container_exec_user_approved_records_tool_decision() {
         .submit(Op::ExecApproval {
             id: "0".into(),
             decision: ReviewDecision::Approved,
-            allow_prefix: None,
         })
         .await
         .unwrap();
@@ -902,7 +901,6 @@ async fn handle_container_exec_user_approved_for_session_records_tool_decision()
         .submit(Op::ExecApproval {
             id: "0".into(),
             decision: ReviewDecision::ApprovedForSession,
-            allow_prefix: None,
         })
         .await
         .unwrap();
@@ -961,7 +959,6 @@ async fn handle_sandbox_error_user_approves_retry_records_tool_decision() {
         .submit(Op::ExecApproval {
             id: "0".into(),
             decision: ReviewDecision::Approved,
-            allow_prefix: None,
         })
         .await
         .unwrap();
@@ -1020,7 +1017,6 @@ async fn handle_container_exec_user_denies_records_tool_decision() {
         .submit(Op::ExecApproval {
             id: "0".into(),
             decision: ReviewDecision::Denied,
-            allow_prefix: None,
         })
         .await
         .unwrap();
@@ -1079,7 +1075,6 @@ async fn handle_sandbox_error_user_approves_for_session_records_tool_decision()
         .submit(Op::ExecApproval {
             id: "0".into(),
             decision: ReviewDecision::ApprovedForSession,
-            allow_prefix: None,
         })
         .await
         .unwrap();
@@ -1139,7 +1134,6 @@ async fn handle_sandbox_error_user_denies_records_tool_decision() {
         .submit(Op::ExecApproval {
             id: "0".into(),
             decision: ReviewDecision::Denied,
-            allow_prefix: None,
         })
         .await
         .unwrap();
diff --git a/codex-rs/mcp-server/src/exec_approval.rs b/codex-rs/mcp-server/src/exec_approval.rs
@@ -150,7 +150,6 @@ async fn on_exec_approval_response(
         .submit(Op::ExecApproval {
             id: event_id,
             decision: response.decision,
-            allow_prefix: None,
         })
         .await
     {
diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs
@@ -143,9 +143,6 @@ pub enum Op {
         id: String,
         /// The user's decision in response to the request.
         decision: ReviewDecision,
-        /// When set, persist this prefix to the execpolicy allow list.
-        #[serde(default, skip_serializing_if = "Option::is_none")]
-        allow_prefix: Option<Vec<String>>,
     },
 
     /// Approve a code patch
@@ -1530,17 +1527,15 @@ pub struct SessionConfiguredEvent {
 }
 
 /// User's decision in response to an ExecApprovalRequest.
-#[derive(
-    Debug, Default, Clone, Copy, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS,
-)]
+#[derive(Debug, Default, Clone, Deserialize, Serialize, PartialEq, Eq, Display, JsonSchema, TS)]
 #[serde(rename_all = "snake_case")]
 pub enum ReviewDecision {
     /// User has approved this command and the agent should execute it.
     Approved,
 
     /// User has approved this command and wants to add the command prefix to
     /// the execpolicy allow list so future matching commands are permitted.
-    ApprovedAllowPrefix,
+    ApprovedAllowPrefix { allow_prefix: Vec<String> },
 
     /// User has approved this command and wants to automatically approve any
     /// future identical instances (`command` and `cwd` match exactly) for the
diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs
@@ -150,9 +150,9 @@ impl ApprovalOverlay {
             return;
         };
         if let Some(variant) = self.current_variant.as_ref() {
-            match (&variant, option.decision) {
+            match (&variant, option.decision.clone()) {
                 (ApprovalVariant::Exec { id, command, .. }, decision) => {
-                    self.handle_exec_decision(id, command, decision, option.allow_prefix.clone());
+                    self.handle_exec_decision(id, command, decision);
                 }
                 (ApprovalVariant::ApplyPatch { id, .. }, decision) => {
                     self.handle_patch_decision(id, decision);
@@ -164,19 +164,14 @@ impl ApprovalOverlay {
         self.advance_queue();
     }
 
-    fn handle_exec_decision(
-        &self,
-        id: &str,
-        command: &[String],
-        decision: ReviewDecision,
-        allow_prefix: Option<Vec<String>>,
-    ) {
-        let cell = history_cell::new_approval_decision_cell(command.to_vec(), decision);
+    fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) {
+        let decision_for_history = decision.clone();
+        let cell =
+            history_cell::new_approval_decision_cell(command.to_vec(), decision_for_history);
         self.app_event_tx.send(AppEvent::InsertHistoryCell(cell));
         self.app_event_tx.send(AppEvent::CodexOp(Op::ExecApproval {
             id: id.to_string(),
             decision,
-            allow_prefix,
         }));
     }
 
@@ -247,7 +242,7 @@ impl BottomPaneView for ApprovalOverlay {
         {
             match &variant {
                 ApprovalVariant::Exec { id, command, .. } => {
-                    self.handle_exec_decision(id, command, ReviewDecision::Abort, None);
+                    self.handle_exec_decision(id, command, ReviewDecision::Abort);
                 }
                 ApprovalVariant::ApplyPatch { id, .. } => {
                     self.handle_patch_decision(id, ReviewDecision::Abort);
@@ -393,7 +388,6 @@ struct ApprovalOption {
     decision: ReviewDecision,
     display_shortcut: Option<KeyBinding>,
     additional_shortcuts: Vec<KeyBinding>,
-    allow_prefix: Option<Vec<String>>,
 }
 
 impl ApprovalOption {
@@ -411,30 +405,28 @@ fn exec_options(allow_prefix: Option<Vec<String>>) -> Vec<ApprovalOption> {
             decision: ReviewDecision::Approved,
             display_shortcut: None,
             additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
-            allow_prefix: None,
         },
         ApprovalOption {
             label: "Yes, and don't ask again for this command".to_string(),
             decision: ReviewDecision::ApprovedForSession,
             display_shortcut: None,
             additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
-            allow_prefix: None,
         },
     ]
     .into_iter()
     .chain(allow_prefix.map(|prefix| ApprovalOption {
         label: "Yes, and don't ask again for commands with this prefix".to_string(),
-        decision: ReviewDecision::ApprovedAllowPrefix,
+        decision: ReviewDecision::ApprovedAllowPrefix {
+            allow_prefix: prefix,
+        },
         display_shortcut: None,
         additional_shortcuts: vec![key_hint::plain(KeyCode::Char('p'))],
-        allow_prefix: Some(prefix),
     }))
     .chain([ApprovalOption {
         label: "No, and tell Codex what to do differently".to_string(),
         decision: ReviewDecision::Abort,
         display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
         additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
-        allow_prefix: None,
     }])
     .collect()
 }
@@ -446,14 +438,12 @@ fn patch_options() -> Vec<ApprovalOption> {
             decision: ReviewDecision::Approved,
             display_shortcut: None,
             additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
-            allow_prefix: None,
         },
         ApprovalOption {
             label: "No, and tell Codex what to do differently".to_string(),
             decision: ReviewDecision::Abort,
             display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
             additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
-            allow_prefix: None,
         },
     ]
 }
@@ -521,14 +511,13 @@ mod tests {
         view.handle_key_event(KeyEvent::new(KeyCode::Char('p'), KeyModifiers::NONE));
         let mut saw_op = false;
         while let Ok(ev) = rx.try_recv() {
-            if let AppEvent::CodexOp(Op::ExecApproval {
-                allow_prefix,
-                decision,
-                ..
-            }) = ev
-            {
-                assert_eq!(decision, ReviewDecision::ApprovedAllowPrefix);
-                assert_eq!(allow_prefix, Some(vec!["echo".to_string()]));
+            if let AppEvent::CodexOp(Op::ExecApproval { decision, .. }) = ev {
+                match decision {
+                    ReviewDecision::ApprovedAllowPrefix { allow_prefix } => {
+                        assert_eq!(allow_prefix, vec!["echo".to_string()]);
+                    }
+                    other => panic!("unexpected decision: {other:?}"),
+                }
                 saw_op = true;
                 break;
             }
diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs
@@ -408,7 +408,7 @@ pub fn new_approval_decision_cell(
                 ],
             )
         }
-        ApprovedAllowPrefix => {
+        ApprovedAllowPrefix { .. } => {
             let snippet = Span::from(exec_snippet(&command)).dim();
             (
                 "✔ ".green(),

Original file line number	Diff line number	Diff line change
`@@ -245,13 +245,7 @@ async fn handle_exec_approval(`
`245`	`245`	`)`
`246`	`246`	`.await;`
`247`	`247`
`248`		`- let _ = codex`
`249`		`- .submit(Op::ExecApproval {`
`250`		`- id,`
`251`		`- decision,`
`252`		`- allow_prefix: None,`
`253`		`- })`
`254`		`- .await;`
	`248`	`+ let _ = codex.submit(Op::ExecApproval { id, decision }).await;`
`255`	`249`	`}`
`256`	`250`
`257`	`251`	`/// Handle an ApplyPatchApprovalRequest by consulting the parent session and replying.`
Original file line number	Diff line number	Diff line change
`@@ -87,14 +87,14 @@ impl ToolOrchestrator {`
`87`	`87`	`};`
`88`	`88`	`let decision = tool.start_approval_async(req, approval_ctx).await;`
`89`	`89`
`90`		`- otel.tool_decision(otel_tn, otel_ci, decision, otel_user.clone());`
	`90`	`+ otel.tool_decision(otel_tn, otel_ci, decision.clone(), otel_user.clone());`
`91`	`91`
`92`	`92`	`match decision {`
`93`	`93`	`ReviewDecision::Denied \| ReviewDecision::Abort => {`
`94`	`94`	`return Err(ToolError::Rejected("rejected by user".to_string()));`
`95`	`95`	`}`
`96`	`96`	`ReviewDecision::Approved`
`97`		`- \| ReviewDecision::ApprovedAllowPrefix`
	`97`	`+ \| ReviewDecision::ApprovedAllowPrefix { .. }`
`98`	`98`	`\| ReviewDecision::ApprovedForSession => {}`
`99`	`99`	`}`
`100`	`100`	`already_approved = true;`
`@@ -169,14 +169,14 @@ impl ToolOrchestrator {`
`169`	`169`	`};`
`170`	`170`
`171`	`171`	`let decision = tool.start_approval_async(req, approval_ctx).await;`
`172`		`- otel.tool_decision(otel_tn, otel_ci, decision, otel_user);`
	`172`	`+ otel.tool_decision(otel_tn, otel_ci, decision.clone(), otel_user);`
`173`	`173`
`174`	`174`	`match decision {`
`175`	`175`	`ReviewDecision::Denied \| ReviewDecision::Abort => {`
`176`	`176`	`return Err(ToolError::Rejected("rejected by user".to_string()));`
`177`	`177`	`}`
`178`	`178`	`ReviewDecision::Approved`
`179`		`- \| ReviewDecision::ApprovedAllowPrefix`
	`179`	`+ \| ReviewDecision::ApprovedAllowPrefix { .. }`
`180`	`180`	`\| ReviewDecision::ApprovedForSession => {}`
`181`	`181`	`}`
`182`	`182`	`}`
Original file line number	Diff line number	Diff line change
`@@ -150,7 +150,6 @@ async fn on_exec_approval_response(`
`150`	`150`	`.submit(Op::ExecApproval {`
`151`	`151`	`id: event_id,`
`152`	`152`	`decision: response.decision,`
`153`		`- allow_prefix: None,`
`154`	`153`	`})`
`155`	`154`	`.await`
`156`	`155`	`{`