feat: scope execve session approvals by approved skill metadata (#12814)

Previous to this change, `determine_action()` would

1. check if `program` is associated with a skill
2. if so, check if `program` is in `execve_session_approvals` to see
whether the user needs to be prompted

This PR flips the order of these checks to try to set us up so that
"session approvals" are always consulted first (which should soon extend
to include session approvals derived from `prefix_rule()`s, as well).

Though to make the new ordering work, we need to record any relevant
metadata to associate with the approval, which in the case of a
skill-based approval is the `SkillMetadata` so that we can derive the
`PermissionProfile` to include with the escalation. (Though as noted by
the `TODO`, this `PermissionProfile` is not honored yet.)

The new `ExecveSessionApproval` struct is used to retain the necessary
metadata.

## What Changed

- Replace the `execve_session_approvals` `HashSet` with a map that
stores an `ExecveSessionApproval` alongside each approved `program`.
- When a user chooses `ApprovedForSession` for a skill script, capture
the matched `SkillMetadata` in the session approval entry.
- Consult that cache before re-running `find_skill()`, and reuse the
originally approved skill metadata and permission profile when allowing
later execve callbacks in the same session.

codex-rs/core/src/codex.rs

@@ -1380,7 +1380,7 @@ impl Session {
             otel_manager,
             models_manager: Arc::clone(&models_manager),
             tool_approvals: Mutex::new(ApprovalStore::default()),
-            execve_session_approvals: RwLock::new(HashSet::new()),
+            execve_session_approvals: RwLock::new(HashMap::new()),
             skills_manager,
             file_watcher,
             agent_control,
@@ -8458,7 +8458,7 @@ mod tests {
             otel_manager: otel_manager.clone(),
             models_manager: Arc::clone(&models_manager),
             tool_approvals: Mutex::new(ApprovalStore::default()),
-            execve_session_approvals: RwLock::new(HashSet::new()),
+            execve_session_approvals: RwLock::new(HashMap::new()),
             skills_manager,
             file_watcher,
             agent_control,
@@ -8617,7 +8617,7 @@ mod tests {
             otel_manager: otel_manager.clone(),
             models_manager: Arc::clone(&models_manager),
             tool_approvals: Mutex::new(ApprovalStore::default()),
-            execve_session_approvals: RwLock::new(HashSet::new()),
+            execve_session_approvals: RwLock::new(HashMap::new()),
             skills_manager,
             file_watcher,
             agent_control,

codex-rs/core/src/state/service.rs

@@ -1,4 +1,4 @@
-use std::collections::HashSet;
+use std::collections::HashMap;
 use std::sync::Arc;
 
 use crate::AuthManager;
@@ -14,6 +14,7 @@ use crate::models_manager::manager::ModelsManager;
 use crate::skills::SkillsManager;
 use crate::state_db::StateDbHandle;
 use crate::tools::network_approval::NetworkApprovalService;
+use crate::tools::runtimes::ExecveSessionApproval;
 use crate::tools::sandboxing::ApprovalStore;
 use crate::unified_exec::UnifiedExecProcessManager;
 use codex_hooks::Hooks;
@@ -45,7 +46,7 @@ pub(crate) struct SessionServices {
     pub(crate) otel_manager: OtelManager,
     pub(crate) tool_approvals: Mutex<ApprovalStore>,
     #[cfg_attr(not(unix), allow(dead_code))]
-    pub(crate) execve_session_approvals: RwLock<HashSet<AbsolutePathBuf>>,
+    pub(crate) execve_session_approvals: RwLock<HashMap<AbsolutePathBuf, ExecveSessionApproval>>,
     pub(crate) skills_manager: Arc<SkillsManager>,
     pub(crate) file_watcher: Arc<FileWatcher>,
     pub(crate) agent_control: AgentControl,

codex-rs/core/src/tools/runtimes/mod.rs

@@ -9,6 +9,7 @@ use crate::path_utils;
 use crate::sandboxing::CommandSpec;
 use crate::sandboxing::SandboxPermissions;
 use crate::shell::Shell;
+use crate::skills::SkillMetadata;
 use crate::tools::sandboxing::ToolError;
 use codex_protocol::models::PermissionProfile;
 use std::collections::HashMap;
@@ -18,6 +19,14 @@ 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 a CommandSpec from a tokenized command line.
 /// Validates that at least a program is present.
 pub(crate) fn build_command_spec(

codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs

@@ -9,6 +9,7 @@ use crate::features::Feature;
 use crate::sandboxing::SandboxPermissions;
 use crate::shell::ShellType;
 use crate::skills::SkillMetadata;
+use crate::tools::runtimes::ExecveSessionApproval;
 use crate::tools::runtimes::build_command_spec;
 use crate::tools::sandboxing::SandboxAttempt;
 use crate::tools::sandboxing::ToolCtx;
@@ -163,8 +164,11 @@ struct CoreShellActionProvider {
     stopwatch: Stopwatch,
 }
 
+#[allow(clippy::large_enum_variant)]
 enum DecisionSource {
-    SkillScript,
+    SkillScript {
+        skill: SkillMetadata,
+    },
     PrefixRule,
     /// Often, this is `is_safe_command()`.
     UnmatchedCommandFallback,
@@ -284,13 +288,21 @@ impl CoreShellActionProvider {
                             // 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 matches!(decision_source, DecisionSource::SkillScript) {
+                            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());
+                                    .insert(
+                                        program.clone(),
+                                        ExecveSessionApproval {
+                                            skill: Some(skill.clone()),
+                                        },
+                                    );
                             }
 
                             if needs_escalation {
@@ -349,6 +361,33 @@ 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"
+            );
+            // TODO(mbolin): We need to include the permissions with the
+            // escalation decision so it can be run with the appropriate
+            // permissions.
+            let _permissions = approval
+                .skill
+                .as_ref()
+                .and_then(|s| s.permission_profile.clone());
+
+            return Ok(EscalateAction::Escalate);
+        }
+
         // 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 {
@@ -356,32 +395,19 @@ impl EscalationPolicy for CoreShellActionProvider {
             // to skills, which means we ignore exec policy rules for those
             // scripts.
             tracing::debug!("Matched {program:?} to skill {skill:?}, prompting for approval");
-            // TODO(mbolin): We should read the permissions associated with the
-            // skill and use those specific permissions in the
-            // EscalateAction::Run case, rather than always escalating when a
-            // skill matches.
             let needs_escalation = true;
-            let is_approved_for_session = self
-                .session
-                .services
-                .execve_session_approvals
-                .read()
-                .await
-                .contains(program);
-            let decision = if is_approved_for_session {
-                Decision::Allow
-            } else {
-                Decision::Prompt
+            let decision_source = DecisionSource::SkillScript {
+                skill: skill.clone(),
             };
             return self
                 .process_decision(
-                    decision,
+                    Decision::Prompt,
                     needs_escalation,
                     program,
                     argv,
                     workdir,
                     skill.permission_profile.clone(),
-                    DecisionSource::SkillScript,
+                    decision_source,
                 )
                 .await;
         }