Ensure shell command skills trigger approval (#12697)

Summary
- detect skill-invoking shell commands based on the original command
string, request approvals when needed, and cache positive decisions per
session
- keep implicit skill invocation emitted after approval and keep skill
approval decline messaging centralized to the shell handler
- expand and adjust skill approval tests to cover shell-based skill
scripts while matching the new detection expectations

Testing
- Not run (not requested)

codex-rs/core/src/codex.rs

@@ -4782,15 +4782,6 @@ pub(crate) async fn run_turn(
         collaboration_mode_kind: turn_context.collaboration_mode.mode,
     });
     sess.send_event(&turn_context, event).await;
-    if turn_context.config.features.enabled(Feature::SkillApproval) {
-        let _ = sess
-            .request_skill_approval(
-                turn_context.as_ref(),
-                turn_context.sub_id.clone(),
-                "test-skill".to_string(),
-            )
-            .await;
-    }
     // TODO(ccunningham): Pre-turn compaction runs before context updates and the
     // new user message are recorded. Estimate pending incoming items (context
     // diffs/full reinjection + user input) and trigger compaction preemptively
@@ -9213,20 +9204,23 @@ mod tests {
 
         let timeout_ms = 1000;
         let sandbox_permissions = SandboxPermissions::RequireEscalated;
+        let command = if cfg!(windows) {
+            vec![
+                "cmd.exe".to_string(),
+                "/C".to_string(),
+                "echo hi".to_string(),
+            ]
+        } else {
+            vec![
+                "/bin/sh".to_string(),
+                "-c".to_string(),
+                "echo hi".to_string(),
+            ]
+        };
         let params = ExecParams {
-            command: if cfg!(windows) {
-                vec![
-                    "cmd.exe".to_string(),
-                    "/C".to_string(),
-                    "echo hi".to_string(),
-                ]
-            } else {
-                vec![
-                    "/bin/sh".to_string(),
-                    "-c".to_string(),
-                    "echo hi".to_string(),
-                ]
-            },
+            command: command.clone(),
+            original_command: shlex::try_join(command.iter().map(String::as_str))
+                .unwrap_or_else(|_| command.join(" ")),
             cwd: turn_context.cwd.clone(),
             expiration: timeout_ms.into(),
             env: HashMap::new(),
@@ -9240,6 +9234,7 @@ mod tests {
         let params2 = ExecParams {
             sandbox_permissions: SandboxPermissions::UseDefault,
             command: params.command.clone(),
+            original_command: params.original_command.clone(),
             cwd: params.cwd.clone(),
             expiration: timeout_ms.into(),
             env: HashMap::new(),

codex-rs/core/src/exec.rs

@@ -62,6 +62,7 @@ pub(crate) const MAX_EXEC_OUTPUT_DELTAS_PER_CALL: usize = 10_000;
 #[derive(Debug)]
 pub struct ExecParams {
     pub command: Vec<String>,
+    pub original_command: String,
     pub cwd: PathBuf,
     pub expiration: ExecExpiration,
     pub env: HashMap<String, String>,
@@ -180,6 +181,7 @@ pub async fn process_exec_tool_call(
 
     let ExecParams {
         command,
+        original_command: _,
         cwd,
         mut env,
         expiration,
@@ -249,6 +251,8 @@ pub(crate) async fn execute_exec_env(
     } = env;
 
     let params = ExecParams {
+        original_command: shlex::try_join(command.iter().map(String::as_str))
+            .unwrap_or_else(|_| command.join(" ")),
         command,
         cwd,
         expiration,
@@ -1121,6 +1125,8 @@ mod tests {
         ];
         let env: HashMap<String, String> = std::env::vars().collect();
         let params = ExecParams {
+            original_command: shlex::try_join(command.iter().map(String::as_str))
+                .unwrap_or_else(|_| command.join(" ")),
             command,
             cwd: std::env::current_dir()?,
             expiration: 500.into(),
@@ -1174,6 +1180,8 @@ mod tests {
         let cancel_token = CancellationToken::new();
         let cancel_tx = cancel_token.clone();
         let params = ExecParams {
+            original_command: shlex::try_join(command.iter().map(String::as_str))
+                .unwrap_or_else(|_| command.join(" ")),
             command,
             cwd: cwd.clone(),
             expiration: ExecExpiration::Cancellation(cancel_token),

codex-rs/core/src/skills/invocation_utils.rs

@@ -7,8 +7,21 @@ use crate::analytics_client::SkillInvocation;
 use crate::analytics_client::build_track_events_context;
 use crate::codex::Session;
 use crate::codex::TurnContext;
+use crate::features::Feature;
 use crate::skills::SkillLoadOutcome;
 use crate::skills::SkillMetadata;
+use codex_protocol::protocol::ReviewDecision;
+use serde::Serialize;
+
+pub(crate) const SKILL_APPROVAL_DECLINED_MESSAGE: &str =
+    "This script is part of the skill and the user declined the skill usage";
+
+#[derive(Debug, Serialize)]
+struct SkillApprovalCacheKey {
+    skill_name: String,
+    skill_path: PathBuf,
+    skill_scope: codex_protocol::protocol::SkillScope,
+}
 
 pub(crate) fn build_implicit_skill_path_indexes(
     skills: Vec<SkillMetadata>,
@@ -41,8 +54,11 @@ fn detect_implicit_skill_invocation_for_command(
     let workdir = normalize_path(workdir.as_path());
     let tokens = tokenize_command(command);
 
-    if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), workdir.as_path())
-    {
+    if let Some(candidate) = detect_implicit_skill_script_invocation_for_tokens(
+        outcome,
+        tokens.as_slice(),
+        workdir.as_path(),
+    ) {
         return Some(candidate);
     }
 
@@ -53,6 +69,82 @@ fn detect_implicit_skill_invocation_for_command(
     None
 }
 
+pub(crate) fn detect_implicit_skill_script_invocation_for_command(
+    outcome: &SkillLoadOutcome,
+    command: &str,
+    workdir: &Path,
+) -> Option<SkillMetadata> {
+    let tokens = tokenize_command(command);
+
+    detect_implicit_skill_script_invocation_for_tokens(outcome, tokens.as_slice(), workdir)
+}
+
+pub(crate) fn detect_implicit_skill_script_invocation_for_tokens(
+    outcome: &SkillLoadOutcome,
+    command: &[String],
+    workdir: &Path,
+) -> Option<SkillMetadata> {
+    detect_skill_script_run(outcome, command, workdir)
+}
+
+fn tokenize_command(command: &str) -> Vec<String> {
+    shlex::split(command).unwrap_or_else(|| {
+        command
+            .split_whitespace()
+            .map(std::string::ToString::to_string)
+            .collect()
+    })
+}
+
+pub(crate) async fn ensure_skill_approval_for_command(
+    sess: &Session,
+    turn_context: &TurnContext,
+    item_id: &str,
+    command: &str,
+    workdir: &Path,
+) -> bool {
+    if !turn_context.features.enabled(Feature::SkillApproval) {
+        return true;
+    }
+
+    let workdir = normalize_path(workdir);
+    let Some(skill) = detect_implicit_skill_script_invocation_for_command(
+        turn_context.turn_skills.outcome.as_ref(),
+        command,
+        workdir.as_path(),
+    ) else {
+        return true;
+    };
+
+    let cache_key = SkillApprovalCacheKey {
+        skill_name: skill.name.clone(),
+        skill_path: skill.path.clone(),
+        skill_scope: skill.scope,
+    };
+    let already_approved = {
+        let store = sess.services.tool_approvals.lock().await;
+        matches!(
+            store.get(&cache_key),
+            Some(ReviewDecision::ApprovedForSession)
+        )
+    };
+    if already_approved {
+        return true;
+    }
+
+    let approved = sess
+        .request_skill_approval(turn_context, item_id.to_string(), skill.name)
+        .await
+        .is_some_and(|response| response.approved);
+    if !approved {
+        return false;
+    }
+
+    let mut store = sess.services.tool_approvals.lock().await;
+    store.put(cache_key, ReviewDecision::ApprovedForSession);
+    true
+}
+
 pub(crate) async fn maybe_emit_implicit_skill_invocation(
     sess: &Session,
     turn_context: &TurnContext,
@@ -115,15 +207,6 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
         );
 }
 
-fn tokenize_command(command: &str) -> Vec<String> {
-    shlex::split(command).unwrap_or_else(|| {
-        command
-            .split_whitespace()
-            .map(std::string::ToString::to_string)
-            .collect()
-    })
-}
-
 fn script_run_token(tokens: &[String]) -> Option<&str> {
     const RUNNERS: [&str; 10] = [
         "python", "python3", "bash", "zsh", "sh", "node", "deno", "ruby", "perl", "pwsh",
@@ -234,6 +317,7 @@ fn normalize_path(path: &Path) -> PathBuf {
 mod tests {
     use super::SkillLoadOutcome;
     use super::SkillMetadata;
+    use super::detect_implicit_skill_script_invocation_for_command;
     use super::detect_skill_doc_read;
     use super::detect_skill_script_run;
     use super::normalize_path;
@@ -353,4 +437,50 @@ mod tests {
             Some("test-skill".to_string())
         );
     }
+
+    #[test]
+    fn implicit_skill_script_invocation_matches_command() {
+        let skill_doc_path = PathBuf::from("/tmp/skill-test/SKILL.md");
+        let scripts_dir = normalize_path(Path::new("/tmp/skill-test/scripts"));
+        let skill = test_skill_metadata(skill_doc_path);
+        let outcome = SkillLoadOutcome {
+            implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])),
+            implicit_skills_by_doc_path: Arc::new(HashMap::new()),
+            ..Default::default()
+        };
+
+        let found = detect_implicit_skill_script_invocation_for_command(
+            &outcome,
+            "python scripts/fetch_comments.py",
+            Path::new("/tmp/skill-test"),
+        );
+
+        assert_eq!(
+            found.map(|value| value.name),
+            Some("test-skill".to_string())
+        );
+    }
+
+    #[test]
+    fn implicit_skill_script_invocation_ignores_doc_reads() {
+        let skill_doc_path = PathBuf::from("/tmp/skill-test/SKILL.md");
+        let normalized_skill_doc_path = normalize_path(skill_doc_path.as_path());
+        let skill = test_skill_metadata(skill_doc_path);
+        let outcome = SkillLoadOutcome {
+            implicit_skills_by_scripts_dir: Arc::new(HashMap::new()),
+            implicit_skills_by_doc_path: Arc::new(HashMap::from([(
+                normalized_skill_doc_path,
+                skill,
+            )])),
+            ..Default::default()
+        };
+
+        let found = detect_implicit_skill_script_invocation_for_command(
+            &outcome,
+            "cat SKILL.md",
+            Path::new("/tmp/skill-test"),
+        );
+
+        assert_eq!(found, None);
+    }
 }

codex-rs/core/src/skills/mod.rs

@@ -14,7 +14,9 @@ pub(crate) use env_var_dependencies::resolve_skill_dependencies_for_turn;
 pub(crate) use injection::SkillInjections;
 pub(crate) use injection::build_skill_injections;
 pub(crate) use injection::collect_explicit_skill_mentions;
+pub(crate) use invocation_utils::SKILL_APPROVAL_DECLINED_MESSAGE;
 pub(crate) use invocation_utils::build_implicit_skill_path_indexes;
+pub(crate) use invocation_utils::ensure_skill_approval_for_command;
 pub(crate) use invocation_utils::maybe_emit_implicit_skill_invocation;
 pub use loader::load_skills;
 pub use manager::SkillsManager;

codex-rs/core/src/tools/handlers/shell.rs

@@ -14,6 +14,8 @@ use crate::function_tool::FunctionCallError;
 use crate::is_safe_command::is_known_safe_command;
 use crate::protocol::ExecCommandSource;
 use crate::shell::Shell;
+use crate::skills::SKILL_APPROVAL_DECLINED_MESSAGE;
+use crate::skills::ensure_skill_approval_for_command;
 use crate::skills::maybe_emit_implicit_skill_invocation;
 use crate::tools::context::ToolInvocation;
 use crate::tools::context::ToolOutput;
@@ -66,6 +68,8 @@ impl ShellHandler {
     ) -> ExecParams {
         ExecParams {
             command: params.command.clone(),
+            original_command: shlex::try_join(params.command.iter().map(String::as_str))
+                .unwrap_or_else(|_| params.command.join(" ")),
             cwd: turn_context.resolve_path(params.workdir.clone()),
             expiration: params.timeout_ms.into(),
             env: create_env(&turn_context.shell_environment_policy, Some(thread_id)),
@@ -116,6 +120,7 @@ impl ShellCommandHandler {
 
         Ok(ExecParams {
             command,
+            original_command: params.command.clone(),
             cwd: turn_context.resolve_path(params.workdir.clone()),
             expiration: params.timeout_ms.into(),
             env: create_env(&turn_context.shell_environment_policy, Some(thread_id)),
@@ -265,13 +270,6 @@ impl ToolHandler for ShellCommandHandler {
         };
 
         let params: ShellCommandToolCallParams = parse_arguments(&arguments)?;
-        maybe_emit_implicit_skill_invocation(
-            session.as_ref(),
-            turn.as_ref(),
-            &params.command,
-            params.workdir.as_deref(),
-        )
-        .await;
         let prefix_rule = params.prefix_rule.clone();
         let exec_params = Self::to_exec_params(
             &params,
@@ -348,6 +346,28 @@ impl ShellHandler {
                 "approval policy is {approval_policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {approval_policy:?}"
             )));
         }
+        let original_command = exec_params.original_command.as_str();
+        if !ensure_skill_approval_for_command(
+            session.as_ref(),
+            turn.as_ref(),
+            &call_id,
+            original_command,
+            exec_params.cwd.as_path(),
+        )
+        .await
+        {
+            return Err(FunctionCallError::RespondToModel(
+                SKILL_APPROVAL_DECLINED_MESSAGE.to_string(),
+            ));
+        }
+        let workdir = exec_params.cwd.to_string_lossy().into_owned();
+        maybe_emit_implicit_skill_invocation(
+            session.as_ref(),
+            turn.as_ref(),
+            original_command,
+            Some(workdir.as_str()),
+        )
+        .await;
 
         // Intercept apply_patch if present.
         if let Some(output) = intercept_apply_patch(