feat: codex-shell-escalation should check skills for excalation policy

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

@@ -8,6 +8,7 @@ use crate::exec::is_likely_sandbox_denied;
 use crate::features::Feature;
 use crate::sandboxing::SandboxPermissions;
 use crate::shell::ShellType;
+use crate::skills::SkillMetadata;
 use crate::tools::runtimes::build_command_spec;
 use crate::tools::sandboxing::SandboxAttempt;
 use crate::tools::sandboxing::ToolCtx;
@@ -174,11 +175,12 @@ impl CoreShellActionProvider {
 
     async fn prompt(
         &self,
-        command: &[String],
+        program: &Path,
+        argv: &[String],
         workdir: &Path,
         stopwatch: &Stopwatch,
     ) -> anyhow::Result<ReviewDecision> {
-        let command = command.to_vec();
+        let command = join_program_and_argv(program, argv);
         let workdir = workdir.to_path_buf();
         let session = self.session.clone();
         let turn = self.turn.clone();
@@ -202,49 +204,41 @@ impl CoreShellActionProvider {
             })
             .await)
     }
-}
 
-#[async_trait::async_trait]
-impl EscalationPolicy for CoreShellActionProvider {
-    async fn determine_action(
+    /// 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: &Path) -> Option<SkillMetadata> {
+        let force_reload = false;
+        let skills_outcome = self
+            .session
+            .services
+            .skills_manager
+            .skills_for_cwd(&self.turn.cwd, force_reload)
+            .await;
+
+        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.starts_with(skill_root.join("scripts")) {
+                return Some(skill);
+            }
+        }
+
+        None
+    }
+
+    async fn process_decision(
         &self,
-        file: &Path,
+        decision: Decision,
+        needs_escalation: bool,
+        program: &Path,
         argv: &[String],
         workdir: &Path,
     ) -> anyhow::Result<EscalateAction> {
-        let command = std::iter::once(file.to_string_lossy().to_string())
-            .chain(argv.iter().cloned())
-            .collect::<Vec<_>>();
-        let (commands, used_complex_parsing) =
-            if let Some(commands) = parse_shell_lc_plain_commands(&command) {
-                (commands, false)
-            } else if let Some(single_command) = parse_shell_lc_single_command_prefix(&command) {
-                (vec![single_command], true)
-            } else {
-                (vec![command.clone()], false)
-            };
-
-        let fallback = |cmd: &[String]| {
-            crate::exec_policy::render_decision_for_unmatched_command(
-                self.approval_policy,
-                &self.sandbox_policy,
-                cmd,
-                self.sandbox_permissions,
-                used_complex_parsing,
-            )
-        };
-        let evaluation = {
-            let policy = self.policy.read().await;
-            policy.check_multiple(commands.iter(), &fallback)
-        };
-        // When true, means the Evaluation was due to *.rules, not the
-        // fallback function.
-        let decision_driven_by_policy =
-            Self::decision_driven_by_policy(&evaluation.matched_rules, evaluation.decision);
-        let needs_escalation =
-            self.sandbox_permissions.requires_escalated_permissions() || decision_driven_by_policy;
-
-        Ok(match evaluation.decision {
+        let action = match decision {
             Decision::Forbidden => EscalateAction::Deny {
                 reason: Some("Execution forbidden by policy".to_string()),
             },
@@ -258,7 +252,7 @@ impl EscalationPolicy for CoreShellActionProvider {
                         reason: Some("Execution forbidden by policy".to_string()),
                     }
                 } else {
-                    match self.prompt(&command, workdir, &self.stopwatch).await? {
+                    match self.prompt(program, argv, workdir, &self.stopwatch).await? {
                         ReviewDecision::Approved
                         | ReviewDecision::ApprovedExecpolicyAmendment { .. }
                         | ReviewDecision::ApprovedForSession => {
@@ -291,8 +285,80 @@ impl EscalationPolicy for CoreShellActionProvider {
                     }
                 }
             }
-            Decision::Allow => EscalateAction::Escalate,
-        })
+            Decision::Allow => {
+                if needs_escalation {
+                    EscalateAction::Escalate
+                } else {
+                    EscalateAction::Run
+                }
+            }
+        };
+        tracing::info!(
+            "Policy decision for command {program:?} is {decision:?}, leading to escalation action {action:?}",
+        );
+        Ok(action)
+    }
+}
+
+#[async_trait::async_trait]
+impl EscalationPolicy for CoreShellActionProvider {
+    async fn determine_action(
+        &self,
+        program: &Path,
+        argv: &[String],
+        workdir: &Path,
+    ) -> anyhow::Result<EscalateAction> {
+        // 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, we always prompt for scripts that look like they belong
+            // to skills, which means we ignore exec policy rules for those
+            // scripts.
+            tracing::info!("Matched {program:?} to skill {skill:?}, prompting for approval");
+            let needs_escalation = self.sandbox_permissions.requires_escalated_permissions();
+            return self
+                .process_decision(Decision::Prompt, needs_escalation, program, argv, workdir)
+                .await;
+        }
+
+        let command = join_program_and_argv(program, argv);
+        let (commands, used_complex_parsing) =
+            if let Some(commands) = parse_shell_lc_plain_commands(&command) {
+                (commands, false)
+            } else if let Some(single_command) = parse_shell_lc_single_command_prefix(&command) {
+                (vec![single_command], true)
+            } else {
+                (vec![command.clone()], false)
+            };
+
+        let fallback = |cmd: &[String]| {
+            crate::exec_policy::render_decision_for_unmatched_command(
+                self.approval_policy,
+                &self.sandbox_policy,
+                cmd,
+                self.sandbox_permissions,
+                used_complex_parsing,
+            )
+        };
+        let evaluation = {
+            let policy = self.policy.read().await;
+            policy.check_multiple(commands.iter(), &fallback)
+        };
+        // When true, means the Evaluation was due to *.rules, not the
+        // fallback function.
+        let decision_driven_by_policy =
+            Self::decision_driven_by_policy(&evaluation.matched_rules, evaluation.decision);
+        let needs_escalation =
+            self.sandbox_permissions.requires_escalated_permissions() || decision_driven_by_policy;
+
+        self.process_decision(
+            evaluation.decision,
+            needs_escalation,
+            program,
+            argv,
+            workdir,
+        )
+        .await
     }
 }
 
@@ -403,6 +469,13 @@ fn map_exec_result(
     Ok(output)
 }
 
+/// Some functions need the full command as a Vec<String>.
+fn join_program_and_argv(program: &Path, argv: &[String]) -> Vec<String> {
+    std::iter::once(program.to_string_lossy().to_string())
+        .chain(argv.iter().cloned())
+        .collect::<Vec<_>>()
+}
+
 #[cfg(test)]
 mod tests {
     use super::ParsedShellCommand;

codex-rs/core/tests/suite/skill_approval.rs

@@ -2,8 +2,10 @@
 
 use anyhow::Result;
 use codex_core::features::Feature;
+use codex_protocol::protocol::AskForApproval;
 use codex_protocol::protocol::EventMsg;
 use codex_protocol::protocol::Op;
+use codex_protocol::protocol::SandboxPolicy;
 use codex_protocol::skill_approval::SkillApprovalResponse;
 use core_test_support::responses::ev_assistant_message;
 use core_test_support::responses::ev_completed;
@@ -59,6 +61,21 @@ fn command_for_script(script_path: &Path) -> Result<String> {
 }
 
 async fn submit_turn(test: &TestCodex, prompt: &str) -> Result<()> {
+    submit_turn_with_policies(
+        test,
+        prompt,
+        AskForApproval::Never,
+        SandboxPolicy::DangerFullAccess,
+    )
+    .await
+}
+
+async fn submit_turn_with_policies(
+    test: &TestCodex,
+    prompt: &str,
+    approval_policy: AskForApproval,
+    sandbox_policy: SandboxPolicy,
+) -> Result<()> {
     let session_model = test.session_configured.model.clone();
     test.codex
         .submit(Op::UserTurn {
@@ -68,8 +85,8 @@ async fn submit_turn(test: &TestCodex, prompt: &str) -> Result<()> {
             }],
             final_output_json_schema: None,
             cwd: test.cwd_path().to_path_buf(),
-            approval_policy: codex_protocol::protocol::AskForApproval::Never,
-            sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess,
+            approval_policy,
+            sandbox_policy,
             model: session_model,
             effort: None,
             summary: codex_protocol::config_types::ReasoningSummary::Auto,
@@ -91,6 +108,65 @@ async fn wait_for_turn_complete_without_skill_approval(test: &TestCodex) {
     .await;
 }
 
+#[cfg(unix)]
+fn write_skill_with_shell_script(home: &Path, name: &str, script_name: &str) -> Result<PathBuf> {
+    use std::os::unix::fs::PermissionsExt;
+
+    let skill_dir = home.join("skills").join(name);
+    let scripts_dir = skill_dir.join("scripts");
+    fs::create_dir_all(&scripts_dir)?;
+    fs::write(
+        skill_dir.join("SKILL.md"),
+        format!("---\nname: {name}\ndescription: {name} skill\n---\n"),
+    )?;
+
+    let script_path = scripts_dir.join(script_name);
+    fs::write(
+        &script_path,
+        "#!/bin/sh\necho 'zsh-fork-stdout'\necho 'zsh-fork-stderr' >&2\n",
+    )?;
+    let mut permissions = fs::metadata(&script_path)?.permissions();
+    permissions.set_mode(0o755);
+    fs::set_permissions(&script_path, permissions)?;
+    Ok(script_path)
+}
+
+#[cfg(unix)]
+fn find_test_zsh_path() -> Result<Option<PathBuf>> {
+    use core_test_support::fetch_dotslash_file;
+
+    let repo_root = codex_utils_cargo_bin::repo_root()?;
+    let dotslash_zsh = repo_root.join("codex-rs/app-server/tests/suite/zsh");
+    if !dotslash_zsh.is_file() {
+        eprintln!(
+            "skipping zsh-fork skill test: shared zsh DotSlash file not found at {}",
+            dotslash_zsh.display()
+        );
+        return Ok(None);
+    }
+
+    match fetch_dotslash_file(&dotslash_zsh, None) {
+        Ok(path) => Ok(Some(path)),
+        Err(error) => {
+            eprintln!("skipping zsh-fork skill test: failed to fetch zsh via dotslash: {error:#}");
+            Ok(None)
+        }
+    }
+}
+
+#[cfg(unix)]
+fn supports_exec_wrapper_intercept(zsh_path: &Path) -> bool {
+    let status = std::process::Command::new(zsh_path)
+        .arg("-fc")
+        .arg("/usr/bin/true")
+        .env("EXEC_WRAPPER", "/usr/bin/false")
+        .status();
+    match status {
+        Ok(status) => !status.success(),
+        Err(_) => false,
+    }
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn skill_approval_event_round_trip_for_shell_command_skill_script_exec() -> Result<()> {
     skip_if_no_network!(Ok(()));
@@ -317,3 +393,120 @@ async fn skill_approval_cache_is_per_skill() -> Result<()> {
 
     Ok(())
 }
+
+#[cfg(unix)]
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn shell_zsh_fork_prompts_for_skill_script_execution() -> Result<()> {
+    use codex_config::Constrained;
+    use codex_protocol::protocol::ReviewDecision;
+
+    skip_if_no_network!(Ok(()));
+
+    let Some(zsh_path) = find_test_zsh_path()? else {
+        return Ok(());
+    };
+    if !supports_exec_wrapper_intercept(&zsh_path) {
+        eprintln!(
+            "skipping zsh-fork skill test: zsh does not support EXEC_WRAPPER intercepts ({})",
+            zsh_path.display()
+        );
+        return Ok(());
+    }
+    let Ok(main_execve_wrapper_exe) = codex_utils_cargo_bin::cargo_bin("codex-execve-wrapper")
+    else {
+        eprintln!("skipping zsh-fork skill test: unable to resolve `codex-execve-wrapper` binary");
+        return Ok(());
+    };
+
+    let server = start_mock_server().await;
+    let tool_call_id = "zsh-fork-skill-call";
+    let mut builder = test_codex()
+        .with_pre_build_hook(|home| {
+            write_skill_with_shell_script(home, "mbolin-test-skill", "hello-mbolin.sh").unwrap();
+        })
+        .with_config(move |config| {
+            config.features.enable(Feature::ShellTool);
+            config.features.enable(Feature::ShellZshFork);
+            config.zsh_path = Some(zsh_path.clone());
+            config.main_execve_wrapper_exe = Some(main_execve_wrapper_exe);
+            config.permissions.allow_login_shell = false;
+            config.permissions.approval_policy = Constrained::allow_any(AskForApproval::OnRequest);
+            config.permissions.sandbox_policy =
+                Constrained::allow_any(SandboxPolicy::new_workspace_write_policy());
+        });
+    let test = builder.build(&server).await?;
+
+    let script_path = fs::canonicalize(
+        test.codex_home_path()
+            .join("skills/mbolin-test-skill/scripts/hello-mbolin.sh"),
+    )?;
+    let script_path_str = script_path.to_string_lossy().into_owned();
+    let command = shlex::try_join([script_path_str.as_str()])?;
+    let arguments = shell_command_arguments(&command)?;
+    let mocks =
+        mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command")
+            .await;
+
+    submit_turn_with_policies(
+        &test,
+        "use $mbolin-test-skill",
+        AskForApproval::OnRequest,
+        SandboxPolicy::new_workspace_write_policy(),
+    )
+    .await?;
+
+    let maybe_approval = wait_for_event_match(test.codex.as_ref(), |event| match event {
+        EventMsg::ExecApprovalRequest(request) => Some(Some(request.clone())),
+        EventMsg::TurnComplete(_) => Some(None),
+        _ => None,
+    })
+    .await;
+    let approval = match maybe_approval {
+        Some(approval) => approval,
+        None => {
+            let call_output = mocks
+                .completion
+                .single_request()
+                .function_call_output(tool_call_id);
+            panic!(
+                "expected exec approval request before completion; function_call_output={call_output:?}"
+            );
+        }
+    };
+    assert_eq!(approval.call_id, tool_call_id);
+    assert_eq!(approval.command.first(), Some(&script_path_str));
+    assert!(
+        approval.command.iter().all(|arg| arg == &script_path_str),
+        "expected approval command to contain only the script path: {:?}",
+        approval.command
+    );
+
+    test.codex
+        .submit(Op::ExecApproval {
+            id: approval.effective_approval_id(),
+            turn_id: None,
+            decision: ReviewDecision::Approved,
+        })
+        .await?;
+
+    wait_for_event(test.codex.as_ref(), |event| {
+        matches!(event, EventMsg::TurnComplete(_))
+    })
+    .await;
+
+    let call_output = mocks
+        .completion
+        .single_request()
+        .function_call_output(tool_call_id);
+    let output = call_output["output"].as_str().unwrap_or_default();
+    assert!(
+        output.contains("zsh-fork-stdout"),
+        "expected stdout marker in function_call_output: {output:?}"
+    );
+    assert!(
+        output.contains("zsh-fork-stderr"),
+        "expected stderr marker in function_call_output: {output:?}"
+    );
+
+    Ok(())
+}

codex-rs/tui/src/chatwidget.rs

@@ -2569,7 +2569,7 @@ impl ChatWidget {
         self.notify(Notification::ExecApprovalRequested { command });
 
         let request = ApprovalRequest::Exec {
-            id: ev.call_id,
+            id: ev.effective_approval_id(),
             command: ev.command,
             reason: ev.reason,
             network_approval_context: ev.network_approval_context,