Clean up permission request approval flow

Replace stringly approval-attempt plumbing with a shared enum, centralize approval decision handling in the orchestrator, and document plus test the reserved PermissionRequest output fields.

Co-authored-by: Codex <noreply@openai.com>

codex-rs/core/src/hook_runtime.rs

@@ -1,6 +1,7 @@
 use std::future::Future;
 use std::sync::Arc;
 
+use codex_hooks::PermissionRequestApprovalAttempt;
 use codex_hooks::PermissionRequestDecision;
 use codex_hooks::PermissionRequestOutcome;
 use codex_hooks::PermissionRequestRequest;
@@ -158,7 +159,7 @@ pub(crate) async fn run_permission_request_hooks(
     turn_context: &Arc<TurnContext>,
     run_id_suffix: String,
     payload: PermissionRequestPayload,
-    approval_attempt: String,
+    approval_attempt: PermissionRequestApprovalAttempt,
     retry_reason: Option<String>,
     network_approval_context: Option<NetworkApprovalContext>,
 ) -> Option<PermissionRequestDecision> {

codex-rs/core/src/tools/orchestrator.rs

@@ -25,6 +25,7 @@ use crate::tools::sandboxing::ToolCtx;
 use crate::tools::sandboxing::ToolError;
 use crate::tools::sandboxing::ToolRuntime;
 use crate::tools::sandboxing::default_exec_approval_requirement;
+use codex_hooks::PermissionRequestApprovalAttempt;
 use codex_hooks::PermissionRequestDecision;
 use codex_otel::SessionTelemetry;
 use codex_otel::ToolDecisionSource;
@@ -46,12 +47,6 @@ pub(crate) struct OrchestratorRunResult<Out> {
     pub deferred_network_approval: Option<DeferredNetworkApproval>,
 }
 
-#[derive(Clone, Copy)]
-enum ApprovalAttempt {
-    Initial,
-    Retry,
-}
-
 struct ApprovalTelemetry<'a> {
     otel: &'a SessionTelemetry,
     tool_name: &'a str,
@@ -162,7 +157,7 @@ impl ToolOrchestrator {
                 let decision = Self::request_approval(
                     tool,
                     req,
-                    ApprovalAttempt::Initial,
+                    PermissionRequestApprovalAttempt::Initial,
                     approval_ctx,
                     turn_ctx,
                     ApprovalTelemetry {
@@ -172,31 +167,12 @@ impl ToolOrchestrator {
                     },
                 )
                 .await?;
-
-                match decision {
-                    ReviewDecision::Denied | ReviewDecision::Abort => {
-                        let reason = if let Some(review_id) = guardian_review_id.as_deref() {
-                            guardian_rejection_message(tool_ctx.session.as_ref(), review_id).await
-                        } else {
-                            "rejected by user".to_string()
-                        };
-                        return Err(ToolError::Rejected(reason));
-                    }
-                    ReviewDecision::TimedOut => {
-                        return Err(ToolError::Rejected(guardian_timeout_message()));
-                    }
-                    ReviewDecision::Approved
-                    | ReviewDecision::ApprovedExecpolicyAmendment { .. }
-                    | ReviewDecision::ApprovedForSession => {}
-                    ReviewDecision::NetworkPolicyAmendment {
-                        network_policy_amendment,
-                    } => match network_policy_amendment.action {
-                        NetworkPolicyRuleAction::Allow => {}
-                        NetworkPolicyRuleAction::Deny => {
-                            return Err(ToolError::Rejected("rejected by user".to_string()));
-                        }
-                    },
-                }
+                Self::enforce_approval_decision(
+                    tool_ctx.session.as_ref(),
+                    guardian_review_id.as_deref(),
+                    decision,
+                )
+                .await?;
                 already_approved = true;
             }
         }
@@ -326,7 +302,7 @@ impl ToolOrchestrator {
                     let decision = Self::request_approval(
                         tool,
                         req,
-                        ApprovalAttempt::Retry,
+                        PermissionRequestApprovalAttempt::Retry,
                         approval_ctx,
                         turn_ctx,
                         ApprovalTelemetry {
@@ -336,32 +312,12 @@ impl ToolOrchestrator {
                         },
                     )
                     .await?;
-
-                    match decision {
-                        ReviewDecision::Denied | ReviewDecision::Abort => {
-                            let reason = if let Some(review_id) = guardian_review_id.as_deref() {
-                                guardian_rejection_message(tool_ctx.session.as_ref(), review_id)
-                                    .await
-                            } else {
-                                "rejected by user".to_string()
-                            };
-                            return Err(ToolError::Rejected(reason));
-                        }
-                        ReviewDecision::TimedOut => {
-                            return Err(ToolError::Rejected(guardian_timeout_message()));
-                        }
-                        ReviewDecision::Approved
-                        | ReviewDecision::ApprovedExecpolicyAmendment { .. }
-                        | ReviewDecision::ApprovedForSession => {}
-                        ReviewDecision::NetworkPolicyAmendment {
-                            network_policy_amendment,
-                        } => match network_policy_amendment.action {
-                            NetworkPolicyRuleAction::Allow => {}
-                            NetworkPolicyRuleAction::Deny => {
-                                return Err(ToolError::Rejected("rejected by user".to_string()));
-                            }
-                        },
-                    }
+                    Self::enforce_approval_decision(
+                        tool_ctx.session.as_ref(),
+                        guardian_review_id.as_deref(),
+                        decision,
+                    )
+                    .await?;
                 }
 
                 let escalated_attempt = SandboxAttempt {
@@ -405,7 +361,7 @@ impl ToolOrchestrator {
     async fn request_approval<Rq, Out, T>(
         tool: &mut T,
         req: &Rq,
-        approval_attempt: ApprovalAttempt,
+        approval_attempt: PermissionRequestApprovalAttempt,
         approval_ctx: ApprovalCtx<'_>,
         turn_ctx: &crate::codex::TurnContext,
         telemetry: ApprovalTelemetry<'_>,
@@ -414,22 +370,16 @@ impl ToolOrchestrator {
         T: ToolRuntime<Rq, Out>,
     {
         if let Some(permission_request) = tool.permission_request_payload(req) {
-            let (approval_attempt_label, run_id_suffix) = match approval_attempt {
-                ApprovalAttempt::Initial => (
-                    "initial".to_string(),
-                    format!("{}:initial", approval_ctx.call_id),
-                ),
-                ApprovalAttempt::Retry => (
-                    "retry".to_string(),
-                    format!("{}:retry", approval_ctx.call_id),
-                ),
-            };
             match run_permission_request_hooks(
                 approval_ctx.session,
                 approval_ctx.turn,
-                run_id_suffix,
+                format!(
+                    "{}:{}",
+                    approval_ctx.call_id,
+                    approval_attempt.as_wire_value()
+                ),
                 permission_request,
-                approval_attempt_label,
+                approval_attempt,
                 approval_ctx.retry_reason.clone(),
                 approval_ctx.network_approval_context.clone(),
             )
@@ -471,6 +421,38 @@ impl ToolOrchestrator {
         );
         Ok(decision)
     }
+
+    // Normalizes approval outcomes from hooks, guardian, and user prompts into
+    // the orchestrator's `Result` flow so both initial and retry approvals use
+    // the same rejection and timeout handling.
+    async fn enforce_approval_decision(
+        session: &crate::codex::Session,
+        guardian_review_id: Option<&str>,
+        decision: ReviewDecision,
+    ) -> Result<(), ToolError> {
+        match decision {
+            ReviewDecision::Denied | ReviewDecision::Abort => {
+                let reason = if let Some(review_id) = guardian_review_id {
+                    guardian_rejection_message(session, review_id).await
+                } else {
+                    "rejected by user".to_string()
+                };
+                Err(ToolError::Rejected(reason))
+            }
+            ReviewDecision::TimedOut => Err(ToolError::Rejected(guardian_timeout_message())),
+            ReviewDecision::Approved
+            | ReviewDecision::ApprovedExecpolicyAmendment { .. }
+            | ReviewDecision::ApprovedForSession => Ok(()),
+            ReviewDecision::NetworkPolicyAmendment {
+                network_policy_amendment,
+            } => match network_policy_amendment.action {
+                NetworkPolicyRuleAction::Allow => Ok(()),
+                NetworkPolicyRuleAction::Deny => {
+                    Err(ToolError::Rejected("rejected by user".to_string()))
+                }
+            },
+        }
+    }
 }
 
 fn build_denial_reason_from_output(_output: &ExecToolCallOutput) -> String {

codex-rs/hooks/schema/generated/permission-request.command.input.schema.json

@@ -72,6 +72,13 @@
       },
       "type": "object"
     },
+    "PermissionRequestApprovalAttempt": {
+      "enum": [
+        "initial",
+        "retry"
+      ],
+      "type": "string"
+    },
     "PermissionRequestApprovalContext": {
       "additionalProperties": false,
       "properties": {
@@ -79,11 +86,7 @@
           "$ref": "#/definitions/PermissionProfile"
         },
         "approval_attempt": {
-          "enum": [
-            "initial",
-            "retry"
-          ],
-          "type": "string"
+          "$ref": "#/definitions/PermissionRequestApprovalAttempt"
         },
         "justification": {
           "type": "string"

codex-rs/hooks/schema/generated/permission-request.command.output.schema.json

@@ -28,6 +28,7 @@
         },
         "interrupt": {
           "default": false,
+          "description": "Reserved for future short-circuiting semantics.\n\nPermissionRequest hooks currently fail closed if this field is `true`.",
           "type": "boolean"
         },
         "message": {
@@ -35,10 +36,12 @@
           "type": "string"
         },
         "updatedInput": {
-          "default": null
+          "default": null,
+          "description": "Reserved for a future input-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present."
         },
         "updatedPermissions": {
-          "default": null
+          "default": null,
+          "description": "Reserved for a future permission-rewrite capability.\n\nPermissionRequest hooks currently fail closed if this field is present."
         }
       },
       "required": [

codex-rs/hooks/src/engine/output_parser.rs

@@ -415,3 +415,80 @@ fn trimmed_reason(reason: &str) -> Option<String> {
         Some(trimmed.to_string())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use pretty_assertions::assert_eq;
+    use serde_json::json;
+
+    use super::parse_permission_request;
+
+    #[test]
+    fn permission_request_rejects_reserved_updated_input_field() {
+        let parsed = parse_permission_request(
+            &json!({
+                "continue": true,
+                "hookSpecificOutput": {
+                    "hookEventName": "PermissionRequest",
+                    "decision": {
+                        "behavior": "allow",
+                        "updatedInput": {}
+                    }
+                }
+            })
+            .to_string(),
+        )
+        .expect("permission request hook output should parse");
+
+        assert_eq!(
+            parsed.invalid_reason,
+            Some("PermissionRequest hook returned unsupported updatedInput".to_string())
+        );
+    }
+
+    #[test]
+    fn permission_request_rejects_reserved_updated_permissions_field() {
+        let parsed = parse_permission_request(
+            &json!({
+                "continue": true,
+                "hookSpecificOutput": {
+                    "hookEventName": "PermissionRequest",
+                    "decision": {
+                        "behavior": "allow",
+                        "updatedPermissions": {}
+                    }
+                }
+            })
+            .to_string(),
+        )
+        .expect("permission request hook output should parse");
+
+        assert_eq!(
+            parsed.invalid_reason,
+            Some("PermissionRequest hook returned unsupported updatedPermissions".to_string())
+        );
+    }
+
+    #[test]
+    fn permission_request_rejects_reserved_interrupt_field() {
+        let parsed = parse_permission_request(
+            &json!({
+                "continue": true,
+                "hookSpecificOutput": {
+                    "hookEventName": "PermissionRequest",
+                    "decision": {
+                        "behavior": "allow",
+                        "interrupt": true
+                    }
+                }
+            })
+            .to_string(),
+        )
+        .expect("permission request hook output should parse");
+
+        assert_eq!(
+            parsed.invalid_reason,
+            Some("PermissionRequest hook returned unsupported interrupt:true".to_string())
+        );
+    }
+}

codex-rs/hooks/src/events/permission_request.rs

@@ -25,6 +25,9 @@ use codex_protocol::protocol::HookOutputEntry;
 use codex_protocol::protocol::HookOutputEntryKind;
 use codex_protocol::protocol::HookRunStatus;
 use codex_protocol::protocol::HookRunSummary;
+use schemars::JsonSchema;
+use serde::Deserialize;
+use serde::Serialize;
 
 use super::common;
 use crate::engine::CommandShell;
@@ -50,11 +53,27 @@ pub struct PermissionRequestRequest {
     pub sandbox_permissions: SandboxPermissions,
     pub additional_permissions: Option<PermissionProfile>,
     pub justification: Option<String>,
-    pub approval_attempt: String,
+    pub approval_attempt: PermissionRequestApprovalAttempt,
     pub retry_reason: Option<String>,
     pub network_approval_context: Option<NetworkApprovalContext>,
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
+#[serde(rename_all = "snake_case")]
+pub enum PermissionRequestApprovalAttempt {
+    Initial,
+    Retry,
+}
+
+impl PermissionRequestApprovalAttempt {
+    pub const fn as_wire_value(self) -> &'static str {
+        match self {
+            Self::Initial => "initial",
+            Self::Retry => "retry",
+        }
+    }
+}
+
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum PermissionRequestDecision {
     Allow,

codex-rs/hooks/src/lib.rs

@@ -5,6 +5,7 @@ mod registry;
 mod schema;
 mod types;
 
+pub use events::permission_request::PermissionRequestApprovalAttempt;
 pub use events::permission_request::PermissionRequestDecision;
 pub use events::permission_request::PermissionRequestOutcome;
 pub use events::permission_request::PermissionRequestRequest;

codex-rs/hooks/src/schema.rs

@@ -16,6 +16,8 @@ use codex_protocol::approvals::NetworkApprovalContext;
 use codex_protocol::models::PermissionProfile;
 use codex_protocol::models::SandboxPermissions;
 
+use crate::events::permission_request::PermissionRequestApprovalAttempt;
+
 const GENERATED_DIR: &str = "generated";
 const POST_TOOL_USE_INPUT_FIXTURE: &str = "post-tool-use.command.input.schema.json";
 const POST_TOOL_USE_OUTPUT_FIXTURE: &str = "post-tool-use.command.output.schema.json";
@@ -142,12 +144,21 @@ pub(crate) struct PermissionRequestHookSpecificOutputWire {
 #[serde(deny_unknown_fields)]
 pub(crate) struct PermissionRequestDecisionWire {
     pub behavior: PermissionRequestBehaviorWire,
+    /// Reserved for a future input-rewrite capability.
+    ///
+    /// PermissionRequest hooks currently fail closed if this field is present.
     #[serde(default)]
     pub updated_input: Option<Value>,
+    /// Reserved for a future permission-rewrite capability.
+    ///
+    /// PermissionRequest hooks currently fail closed if this field is present.
     #[serde(default)]
     pub updated_permissions: Option<Value>,
     #[serde(default)]
     pub message: Option<String>,
+    /// Reserved for future short-circuiting semantics.
+    ///
+    /// PermissionRequest hooks currently fail closed if this field is `true`.
     #[serde(default)]
     pub interrupt: bool,
 }
@@ -245,8 +256,7 @@ pub(crate) struct PermissionRequestApprovalContext {
     pub sandbox_permissions: SandboxPermissions,
     pub additional_permissions: Option<PermissionProfile>,
     pub justification: Option<String>,
-    #[schemars(schema_with = "permission_request_approval_attempt_schema")]
-    pub approval_attempt: String,
+    pub approval_attempt: PermissionRequestApprovalAttempt,
     pub retry_reason: Option<String>,
     pub network_approval_context: Option<NetworkApprovalContext>,
 }
@@ -571,10 +581,6 @@ fn permission_request_tool_name_schema(_gen: &mut SchemaGenerator) -> Schema {
     string_const_schema("Bash")
 }
 
-fn permission_request_approval_attempt_schema(_gen: &mut SchemaGenerator) -> Schema {
-    string_enum_schema(&["initial", "retry"])
-}
-
 fn user_prompt_submit_hook_event_name_schema(_gen: &mut SchemaGenerator) -> Schema {
     string_const_schema("UserPromptSubmit")
 }