From 59398125f67d570dcac522fc61024b9e278173fa Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 24 Feb 2026 23:51:26 -0800 Subject: [PATCH] feat: zsh-fork forces scripts/**/* for skills to trigger a prompt (#12730) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Direct skill-script matches force `Decision::Prompt`, so skill-backed scripts require explicit approval before they run. (Note "allow for session" is not supported in this PR, but will be done in a follow-up.) In the process of implementing this, I fixed an important bug: `ShellZshFork` is supposed to keep ordinary allowed execs on the client-side `Run` path so later `execve()` calls are still intercepted and reviewed. After the shell-escalation port, `Decision::Allow` still mapped to `Escalate`, which moved `zsh` to server-side execution too early. That broke the intended flow for skill-backed scripts and made the approval prompt depend on the wrong execution path. ## What changed - In `codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs`, `Decision::Allow` now returns `Run` unless escalation is actually required. - Removed the zsh-specific `argv[0]` fallback. With the `Allow -> Run` fix in place, zsh's later `execve()` of the script is intercepted normally, so the skill match happens on the script path itself. - Kept the skill-path handling in `determine_action()` focused on the direct `program` match path. ## Verification - Updated `shell_zsh_fork_prompts_for_skill_script_execution` in `codex-rs/core/tests/suite/skill_approval.rs` (gated behind `cfg(unix)`) to: - run under `SandboxPolicy::new_workspace_write_policy()` instead of `DangerFullAccess` - assert the approval command contains only the script path - assert the approved run returns both stdout and stderr markers in the shell output - Ran `cargo test -p codex-core shell_zsh_fork_prompts_for_skill_script_execution -- --nocapture` ## Manual Testing Run the dev build: ``` just codex --config zsh_path=/Users/mbolin/code/codex2/codex-rs/app-server/tests/suite/zsh --enable shell_zsh_fork ``` I have created `/Users/mbolin/.agents/skills/mbolin-test-skill` with: ``` ├── scripts │ └── hello-mbolin.sh └── SKILL.md ``` The skill: ``` --- name: mbolin-test-skill description: Used to exercise various features of skills. --- When this skill is invoked, run the `hello-mbolin.sh` script and report the output. ``` The script: ``` set -e # Note this script will fail if run with network disabled. curl --location openai.com ``` Use `$mbolin-test-skill` to invoke the skill manually and verify that I get prompted to run `hello-mbolin.sh`. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/12730). * #12750 * __->__ #12730 --- .../tools/runtimes/shell/unix_escalation.rs | 189 ++++++++++++---- codex-rs/core/tests/suite/skill_approval.rs | 214 +++++++++++++++++- 2 files changed, 356 insertions(+), 47 deletions(-) diff --git a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs index b7d9b5a25..796506e13 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/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 { - 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 { + 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 { - let command = std::iter::once(file.to_string_lossy().to_string()) - .chain(argv.iter().cloned()) - .collect::>(); - 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,88 @@ impl EscalationPolicy for CoreShellActionProvider { } } } - Decision::Allow => EscalateAction::Escalate, - }) + Decision::Allow => { + if needs_escalation { + EscalateAction::Escalate + } else { + EscalateAction::Run + } + } + }; + tracing::debug!( + "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 { + tracing::debug!( + "Determining escalation action for command {program:?} with args {argv:?} in {workdir:?}" + ); + + // 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::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; + 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,14 +477,28 @@ fn map_exec_result( Ok(output) } +/// Convert an intercepted exec `(program, argv)` into a command vector suitable +/// for display and policy parsing. +/// +/// The intercepted `argv` includes `argv[0]`, but once we have normalized the +/// executable path in `program`, we should replace the original `argv[0]` +/// rather than duplicating it as an apparent user argument. +fn join_program_and_argv(program: &Path, argv: &[String]) -> Vec { + std::iter::once(program.to_string_lossy().to_string()) + .chain(argv.iter().skip(1).cloned()) + .collect::>() +} + #[cfg(test)] mod tests { use super::ParsedShellCommand; use super::extract_shell_script; + use super::join_program_and_argv; use super::map_exec_result; use crate::exec::SandboxType; use codex_shell_escalation::ExecResult; use pretty_assertions::assert_eq; + use std::path::Path; use std::time::Duration; #[test] @@ -431,6 +519,21 @@ mod tests { ); } + #[test] + fn join_program_and_argv_replaces_original_argv_zero() { + assert_eq!( + join_program_and_argv( + Path::new("/tmp/tool"), + &["./tool".into(), "--flag".into(), "value".into()], + ), + vec!["/tmp/tool", "--flag", "value"] + ); + assert_eq!( + join_program_and_argv(Path::new("/tmp/tool"), &["./tool".into()]), + vec!["/tmp/tool"] + ); + } + #[test] fn map_exec_result_preserves_stdout_and_stderr() { let out = map_exec_result( diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index de0b57886..e64ca4c19 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/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; @@ -30,7 +32,13 @@ fn write_skill_with_script(home: &Path, name: &str, script_body: &str) -> Result fs::create_dir_all(&scripts_dir)?; fs::write( skill_dir.join("SKILL.md"), - format!("---\nname: {name}\ndescription: {name} skill\n---\n"), + format!( + r#"--- +name: {name} +description: {name} skill +--- +"# + ), )?; let script_path = scripts_dir.join("run.py"); fs::write(&script_path, script_body)?; @@ -59,6 +67,21 @@ fn command_for_script(script_path: &Path) -> Result { } 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 +91,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 +114,74 @@ 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 { + 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!( + r#"--- +name: {name} +description: {name} skill +--- +"# + ), + )?; + + let script_path = scripts_dir.join(script_name); + fs::write( + &script_path, + r#"#!/bin/sh +echo 'zsh-fork-stdout' +echo 'zsh-fork-stderr' >&2 +"#, + )?; + 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> { + 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(())); @@ -170,7 +261,10 @@ async fn skill_approval_decline_blocks_execution() -> Result<()> { let marker_path = home.join(marker_name); let marker_path = marker_path.to_string_lossy(); let script_body = format!( - "from pathlib import Path\nPath({marker_path:?}).write_text('ran')\nprint('ran')\n" + r#"from pathlib import Path +Path({marker_path:?}).write_text('ran') +print('ran') +"# ); write_skill_with_script(home, "demo", &script_body).unwrap(); }) @@ -317,3 +411,115 @@ 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, vec![script_path_str.clone()]); + + 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(()) +}