core: remove special execve handling for skill scripts (#15812)

bolinfest · web-flow · commit 01fa4f0212db · 2026-03-26T07:46:04.000-07:00
diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs
@@ -1870,7 +1870,6 @@ impl Session {
             session_telemetry,
             models_manager: Arc::clone(&models_manager),
             tool_approvals: Mutex::new(ApprovalStore::default()),
-            execve_session_approvals: RwLock::new(HashMap::new()),
             skills_manager,
             plugins_manager: Arc::clone(&plugins_manager),
             mcp_manager: Arc::clone(&mcp_manager),
diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs
@@ -2683,7 +2683,6 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
         session_telemetry: session_telemetry.clone(),
         models_manager: Arc::clone(&models_manager),
         tool_approvals: Mutex::new(ApprovalStore::default()),
-        execve_session_approvals: RwLock::new(HashMap::new()),
         skills_manager,
         plugins_manager,
         mcp_manager,
@@ -3521,7 +3520,6 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
         session_telemetry: session_telemetry.clone(),
         models_manager: Arc::clone(&models_manager),
         tool_approvals: Mutex::new(ApprovalStore::default()),
-        execve_session_approvals: RwLock::new(HashMap::new()),
         skills_manager,
         plugins_manager,
         mcp_manager,
diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs
@@ -1,4 +1,3 @@
-use std::collections::HashMap;
 use std::sync::Arc;
 
 use crate::AuthManager;
@@ -16,14 +15,12 @@ use crate::skills_watcher::SkillsWatcher;
 use crate::state_db::StateDbHandle;
 use crate::tools::code_mode::CodeModeService;
 use crate::tools::network_approval::NetworkApprovalService;
-use crate::tools::runtimes::ExecveSessionApproval;
 use crate::tools::sandboxing::ApprovalStore;
 use crate::unified_exec::UnifiedExecProcessManager;
 use codex_analytics::AnalyticsEventsClient;
 use codex_exec_server::Environment;
 use codex_hooks::Hooks;
 use codex_otel::SessionTelemetry;
-use codex_utils_absolute_path::AbsolutePathBuf;
 use std::path::PathBuf;
 use tokio::sync::Mutex;
 use tokio::sync::RwLock;
@@ -49,8 +46,6 @@ pub(crate) struct SessionServices {
     pub(crate) models_manager: Arc<ModelsManager>,
     pub(crate) session_telemetry: SessionTelemetry,
     pub(crate) tool_approvals: Mutex<ApprovalStore>,
-    #[cfg_attr(not(unix), allow(dead_code))]
-    pub(crate) execve_session_approvals: RwLock<HashMap<AbsolutePathBuf, ExecveSessionApproval>>,
     pub(crate) skills_manager: Arc<SkillsManager>,
     pub(crate) plugins_manager: Arc<PluginsManager>,
     pub(crate) mcp_manager: Arc<McpManager>,
diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs
@@ -4,7 +4,6 @@ Module: runtimes
 Concrete ToolRuntime implementations for specific tools. Each runtime stays
 small and focused and reuses the orchestrator for approvals + sandbox + retry.
 */
-use crate::SkillMetadata;
 use crate::path_utils;
 use crate::shell::Shell;
 use crate::tools::sandboxing::ToolError;
@@ -17,14 +16,6 @@ pub mod apply_patch;
 pub mod shell;
 pub mod unified_exec;
 
-#[derive(Debug, Clone)]
-pub(crate) struct ExecveSessionApproval {
-    /// If this execve session approval is associated with a skill script, this
-    /// field contains metadata about the skill.
-    #[cfg_attr(not(unix), allow(dead_code))]
-    pub skill: Option<SkillMetadata>,
-}
-
 /// Shared helper to construct sandbox transform inputs from a tokenized command line.
 /// Validates that at least a program is present.
 pub(crate) fn build_sandbox_command(
diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
@@ -1,5 +1,4 @@
 use super::ShellRequest;
-use crate::SkillMetadata;
 use crate::error::CodexErr;
 use crate::error::SandboxErr;
 use crate::exec::ExecCapturePolicy;
@@ -13,8 +12,6 @@ use crate::sandboxing::ExecOptions;
 use crate::sandboxing::ExecRequest;
 use crate::sandboxing::SandboxPermissions;
 use crate::shell::ShellType;
-use crate::skills_load_input_from_config;
-use crate::tools::runtimes::ExecveSessionApproval;
 use crate::tools::runtimes::build_sandbox_command;
 use crate::tools::sandboxing::SandboxAttempt;
 use crate::tools::sandboxing::ToolCtx;
@@ -31,7 +28,6 @@ use codex_protocol::models::PermissionProfile;
 use codex_protocol::permissions::FileSystemSandboxPolicy;
 use codex_protocol::permissions::NetworkSandboxPolicy;
 use codex_protocol::protocol::AskForApproval;
-use codex_protocol::protocol::ExecApprovalRequestSkillMetadata;
 use codex_protocol::protocol::NetworkPolicyRuleAction;
 use codex_protocol::protocol::ReviewDecision;
 use codex_protocol::protocol::SandboxPolicy;
@@ -74,9 +70,6 @@ const REJECT_SANDBOX_APPROVAL_REASON: &str =
     "approval required by policy, but AskForApproval::Granular.sandbox_approval is false";
 const REJECT_RULES_APPROVAL_REASON: &str =
     "approval required by policy rule, but AskForApproval::Granular.rules is false";
-const REJECT_SKILL_APPROVAL_REASON: &str =
-    "approval required by skill, but AskForApproval::Granular.skill_approval is false";
-
 fn approval_sandbox_permissions(
     sandbox_permissions: SandboxPermissions,
     additional_permissions_preapproved: bool,
@@ -327,9 +320,6 @@ struct CoreShellActionProvider {
 
 #[allow(clippy::large_enum_variant)]
 enum DecisionSource {
-    SkillScript {
-        skill: SkillMetadata,
-    },
     PrefixRule,
     /// Often, this is `is_safe_command()`.
     UnmatchedCommandFallback,
@@ -341,11 +331,6 @@ fn execve_prompt_is_rejected_by_policy(
 ) -> Option<&'static str> {
     match (approval_policy, decision_source) {
         (AskForApproval::Never, _) => Some(PROMPT_CONFLICT_REASON),
-        (AskForApproval::Granular(granular_config), DecisionSource::SkillScript { .. })
-            if !granular_config.allows_skill_approval() =>
-        {
-            Some(REJECT_SKILL_APPROVAL_REASON)
-        }
         (AskForApproval::Granular(granular_config), DecisionSource::PrefixRule)
             if !granular_config.allows_rules_approval() =>
         {
@@ -397,25 +382,13 @@ impl CoreShellActionProvider {
         }
     }
 
-    fn skill_escalation_execution(skill: &SkillMetadata) -> EscalationExecution {
-        let permission_profile = skill.permission_profile.clone().unwrap_or_default();
-        if permission_profile.is_empty() {
-            EscalationExecution::TurnDefault
-        } else {
-            EscalationExecution::Permissions(EscalationPermissions::PermissionProfile(
-                permission_profile,
-            ))
-        }
-    }
-
     async fn prompt(
         &self,
         program: &AbsolutePathBuf,
         argv: &[String],
         workdir: &AbsolutePathBuf,
         stopwatch: &Stopwatch,
         additional_permissions: Option<PermissionProfile>,
-        decision_source: &DecisionSource,
     ) -> anyhow::Result<ReviewDecision> {
         let command = join_program_and_argv(program, argv);
         let workdir = workdir.to_path_buf();
@@ -442,28 +415,6 @@ impl CoreShellActionProvider {
                     )
                     .await;
                 }
-                let available_decisions = vec![
-                    Some(ReviewDecision::Approved),
-                    // Currently, ApprovedForSession is only honored for skills,
-                    // so only offer it for skill script approvals.
-                    if matches!(decision_source, DecisionSource::SkillScript { .. }) {
-                        Some(ReviewDecision::ApprovedForSession)
-                    } else {
-                        None
-                    },
-                    Some(ReviewDecision::Abort),
-                ]
-                .into_iter()
-                .flatten()
-                .collect();
-                let skill_metadata = match decision_source {
-                    DecisionSource::SkillScript { skill } => {
-                        Some(ExecApprovalRequestSkillMetadata {
-                            path_to_skills_md: skill.path_to_skills_md.clone(),
-                        })
-                    }
-                    DecisionSource::PrefixRule | DecisionSource::UnmatchedCommandFallback => None,
-                };
                 session
                     .request_command_approval(
                         &turn,
@@ -475,48 +426,14 @@ impl CoreShellActionProvider {
                         /*network_approval_context*/ None,
                         /*proposed_execpolicy_amendment*/ None,
                         additional_permissions,
-                        skill_metadata,
-                        Some(available_decisions),
+                        /*skill_metadata*/ None,
+                        Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]),
                     )
                     .await
             })
             .await)
     }
 
-    /// Because we should be intercepting execve(2) calls, `program` should be
-    /// an absolute path. The idea is that we check to see whether it matches
-    /// any skills.
-    async fn find_skill(&self, program: &AbsolutePathBuf) -> Option<SkillMetadata> {
-        let force_reload = false;
-        let turn_config = self.turn.config.as_ref();
-        let plugin_outcome = self
-            .session
-            .services
-            .plugins_manager
-            .plugins_for_config(turn_config);
-        let effective_skill_roots = plugin_outcome.effective_skill_roots();
-        let skills_input = skills_load_input_from_config(turn_config, effective_skill_roots);
-        let skills_outcome = self
-            .session
-            .services
-            .skills_manager
-            .skills_for_cwd(&skills_input, force_reload)
-            .await;
-
-        let program_path = program.as_path();
-        for skill in skills_outcome.skills {
-            // We intentionally ignore "enabled" status here for now.
-            let Some(skill_root) = skill.path_to_skills_md.parent() else {
-                continue;
-            };
-            if program_path.starts_with(skill_root.join("scripts")) {
-                return Some(skill);
-            }
-        }
-
-        None
-    }
-
     #[allow(clippy::too_many_arguments)]
     async fn process_decision(
         &self,
@@ -540,51 +457,18 @@ impl CoreShellActionProvider {
                     EscalationDecision::deny(Some("Execution forbidden by policy".to_string()))
                 } else {
                     match self
-                        .prompt(
-                            program,
-                            argv,
-                            workdir,
-                            &self.stopwatch,
-                            prompt_permissions,
-                            &decision_source,
-                        )
+                        .prompt(program, argv, workdir, &self.stopwatch, prompt_permissions)
                         .await?
                     {
                         ReviewDecision::Approved
+                        | ReviewDecision::ApprovedForSession
                         | ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
                             if needs_escalation {
                                 EscalationDecision::escalate(escalation_execution.clone())
                             } else {
                                 EscalationDecision::run()
                             }
                         }
-                        ReviewDecision::ApprovedForSession => {
-                            // Currently, we only add session approvals for
-                            // skill scripts because we are storing only the
-                            // `program` whereas prefix rules may be restricted by a longer prefix.
-                            if let DecisionSource::SkillScript { skill } = decision_source {
-                                tracing::debug!(
-                                    "Adding session approval for {program:?} due to user approval of skill script {skill:?}"
-                                );
-                                self.session
-                                    .services
-                                    .execve_session_approvals
-                                    .write()
-                                    .await
-                                    .insert(
-                                        program.clone(),
-                                        ExecveSessionApproval {
-                                            skill: Some(skill.clone()),
-                                        },
-                                    );
-                            }
-
-                            if needs_escalation {
-                                EscalationDecision::escalate(escalation_execution.clone())
-                            } else {
-                                EscalationDecision::run()
-                            }
-                        }
                         ReviewDecision::NetworkPolicyAmendment {
                             network_policy_amendment,
                         } => match network_policy_amendment.action {
@@ -641,69 +525,6 @@ impl EscalationPolicy for CoreShellActionProvider {
             "Determining escalation action for command {program:?} with args {argv:?} in {workdir:?}"
         );
 
-        // Check to see whether `program` has an existing entry in
-        // `execve_session_approvals`. If so, we can skip policy checks and user
-        // prompts and go straight to allowing execution.
-        let approval = {
-            self.session
-                .services
-                .execve_session_approvals
-                .read()
-                .await
-                .get(program)
-                .cloned()
-        };
-        if let Some(approval) = approval {
-            tracing::debug!(
-                "Found session approval for {program:?}, allowing execution without further checks"
-            );
-            let execution = approval
-                .skill
-                .as_ref()
-                .map(Self::skill_escalation_execution)
-                .unwrap_or(EscalationExecution::TurnDefault);
-
-            return Ok(EscalationDecision::escalate(execution));
-        }
-
-        // In the usual case, the execve wrapper reports the command being
-        // executed in `program`, so a direct skill lookup is sufficient.
-        if let Some(skill) = self.find_skill(program).await {
-            // For now, scripts that look like they belong to skills bypass
-            // general exec policy evaluation. Permissionless skills inherit the
-            // turn sandbox directly; skills with declared permissions still
-            // prompt here before applying their permission profile.
-            let prompt_permissions = skill.permission_profile.clone();
-            if prompt_permissions
-                .as_ref()
-                .is_none_or(PermissionProfile::is_empty)
-            {
-                tracing::debug!(
-                    "Matched {program:?} to permissionless skill {skill:?}, inheriting turn sandbox"
-                );
-                return Ok(EscalationDecision::escalate(
-                    EscalationExecution::TurnDefault,
-                ));
-            }
-            tracing::debug!("Matched {program:?} to skill {skill:?}, prompting for approval");
-            let needs_escalation = true;
-            let decision_source = DecisionSource::SkillScript {
-                skill: skill.clone(),
-            };
-            return self
-                .process_decision(
-                    Decision::Prompt,
-                    needs_escalation,
-                    program,
-                    argv,
-                    workdir,
-                    prompt_permissions,
-                    Self::skill_escalation_execution(&skill),
-                    decision_source,
-                )
-                .await;
-        }
-
         let evaluation = {
             let policy = self.policy.read().await;
             evaluate_intercepted_exec_policy(
@@ -746,7 +567,6 @@ impl EscalationPolicy for CoreShellActionProvider {
                     .macos_seatbelt_profile_extensions
                     .as_ref(),
             ),
-            DecisionSource::SkillScript { .. } => unreachable!("handled above"),
         };
         self.process_decision(
             evaluation.decision,
diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs
diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs
