diff --git a/codex-rs/app-server-test-client/src/lib.rs b/codex-rs/app-server-test-client/src/lib.rs index 7b846c80f1..552513ab27 100644 --- a/codex-rs/app-server-test-client/src/lib.rs +++ b/codex-rs/app-server-test-client/src/lib.rs @@ -2,6 +2,7 @@ use std::collections::VecDeque; use std::ffi::OsString; use std::fs; use std::fs::OpenOptions; +use std::io; use std::io::BufRead; use std::io::BufReader; use std::io::Write; @@ -32,6 +33,7 @@ use codex_app_server_protocol::CommandExecutionRequestApprovalParams; use codex_app_server_protocol::CommandExecutionRequestApprovalResponse; use codex_app_server_protocol::CommandExecutionStatus; use codex_app_server_protocol::DynamicToolSpec; +use codex_app_server_protocol::ExecPolicyAmendment; use codex_app_server_protocol::FileChangeApprovalDecision; use codex_app_server_protocol::FileChangeRequestApprovalParams; use codex_app_server_protocol::FileChangeRequestApprovalResponse; @@ -143,10 +145,28 @@ struct Cli { #[arg(long, value_name = "json-or-@file", global = true)] dynamic_tools: Option, + /// Attach a skill input item to V2 turn/start requests. + /// + /// Must be paired with --skill-path. + #[arg(long, value_name = "skill-name", global = true)] + skill_name: Option, + + /// Path to the SKILL.md file for --skill-name. + /// + /// Must be paired with --skill-name. + #[arg(long, value_name = "path-to-skill-md", global = true)] + skill_path: Option, + #[command(subcommand)] command: CliCommand, } +#[derive(Clone, Debug)] +struct SkillSelection { + name: String, + path: PathBuf, +} + #[derive(Subcommand)] enum CliCommand { /// Start `codex app-server` on a websocket endpoint in the background. @@ -241,25 +261,36 @@ pub fn run() -> Result<()> { url, config_overrides, dynamic_tools, + skill_name, + skill_path, command, } = Cli::parse(); let dynamic_tools = parse_dynamic_tools_arg(&dynamic_tools)?; + let skill_selection = parse_skill_selection(skill_name, skill_path)?; match command { CliCommand::Serve { listen, kill } => { ensure_dynamic_tools_unused(&dynamic_tools, "serve")?; + ensure_skill_unused(&skill_selection, "serve")?; let codex_bin = codex_bin.unwrap_or_else(|| PathBuf::from("codex")); serve(&codex_bin, &config_overrides, &listen, kill) } CliCommand::SendMessage { user_message } => { ensure_dynamic_tools_unused(&dynamic_tools, "send-message")?; + ensure_skill_unused(&skill_selection, "send-message")?; let endpoint = resolve_endpoint(codex_bin, url)?; send_message(&endpoint, &config_overrides, user_message) } CliCommand::SendMessageV2 { user_message } => { let endpoint = resolve_endpoint(codex_bin, url)?; - send_message_v2_endpoint(&endpoint, &config_overrides, user_message, &dynamic_tools) + send_message_v2_endpoint( + &endpoint, + &config_overrides, + user_message, + &dynamic_tools, + skill_selection.as_ref(), + ) } CliCommand::ResumeMessageV2 { thread_id, @@ -272,24 +303,43 @@ pub fn run() -> Result<()> { thread_id, user_message, &dynamic_tools, + skill_selection.as_ref(), ) } CliCommand::ThreadResume { thread_id } => { ensure_dynamic_tools_unused(&dynamic_tools, "thread-resume")?; + ensure_skill_unused(&skill_selection, "thread-resume")?; let endpoint = resolve_endpoint(codex_bin, url)?; thread_resume_follow(&endpoint, &config_overrides, thread_id) } CliCommand::TriggerCmdApproval { user_message } => { let endpoint = resolve_endpoint(codex_bin, url)?; - trigger_cmd_approval(&endpoint, &config_overrides, user_message, &dynamic_tools) + trigger_cmd_approval( + &endpoint, + &config_overrides, + user_message, + &dynamic_tools, + skill_selection.as_ref(), + ) } CliCommand::TriggerPatchApproval { user_message } => { let endpoint = resolve_endpoint(codex_bin, url)?; - trigger_patch_approval(&endpoint, &config_overrides, user_message, &dynamic_tools) + trigger_patch_approval( + &endpoint, + &config_overrides, + user_message, + &dynamic_tools, + skill_selection.as_ref(), + ) } CliCommand::NoTriggerCmdApproval => { let endpoint = resolve_endpoint(codex_bin, url)?; - no_trigger_cmd_approval(&endpoint, &config_overrides, &dynamic_tools) + no_trigger_cmd_approval( + &endpoint, + &config_overrides, + &dynamic_tools, + skill_selection.as_ref(), + ) } CliCommand::SendFollowUpV2 { first_message, @@ -302,6 +352,7 @@ pub fn run() -> Result<()> { first_message, follow_up_message, &dynamic_tools, + skill_selection.as_ref(), ) } CliCommand::TriggerZshForkMultiCmdApproval { @@ -321,21 +372,25 @@ pub fn run() -> Result<()> { } CliCommand::TestLogin => { ensure_dynamic_tools_unused(&dynamic_tools, "test-login")?; + ensure_skill_unused(&skill_selection, "test-login")?; let endpoint = resolve_endpoint(codex_bin, url)?; test_login(&endpoint, &config_overrides) } CliCommand::GetAccountRateLimits => { ensure_dynamic_tools_unused(&dynamic_tools, "get-account-rate-limits")?; + ensure_skill_unused(&skill_selection, "get-account-rate-limits")?; let endpoint = resolve_endpoint(codex_bin, url)?; get_account_rate_limits(&endpoint, &config_overrides) } CliCommand::ModelList => { ensure_dynamic_tools_unused(&dynamic_tools, "model-list")?; + ensure_skill_unused(&skill_selection, "model-list")?; let endpoint = resolve_endpoint(codex_bin, url)?; model_list(&endpoint, &config_overrides) } CliCommand::ThreadList { limit } => { ensure_dynamic_tools_unused(&dynamic_tools, "thread-list")?; + ensure_skill_unused(&skill_selection, "thread-list")?; let endpoint = resolve_endpoint(codex_bin, url)?; thread_list(&endpoint, &config_overrides, limit) } @@ -505,7 +560,13 @@ pub fn send_message_v2( dynamic_tools: &Option>, ) -> Result<()> { let endpoint = Endpoint::SpawnCodex(codex_bin.to_path_buf()); - send_message_v2_endpoint(&endpoint, config_overrides, user_message, dynamic_tools) + send_message_v2_endpoint( + &endpoint, + config_overrides, + user_message, + dynamic_tools, + None, + ) } fn send_message_v2_endpoint( @@ -513,6 +574,7 @@ fn send_message_v2_endpoint( config_overrides: &[String], user_message: String, dynamic_tools: &Option>, + skill_selection: Option<&SkillSelection>, ) -> Result<()> { send_message_v2_with_policies( endpoint, @@ -521,6 +583,7 @@ fn send_message_v2_endpoint( None, None, dynamic_tools, + skill_selection, ) } @@ -625,6 +688,7 @@ fn resume_message_v2( thread_id: String, user_message: String, dynamic_tools: &Option>, + skill_selection: Option<&SkillSelection>, ) -> Result<()> { ensure_dynamic_tools_unused(dynamic_tools, "resume-message-v2")?; @@ -641,10 +705,7 @@ fn resume_message_v2( let turn_response = client.turn_start(TurnStartParams { thread_id: resume_response.thread.id.clone(), - input: vec![V2UserInput::Text { - text: user_message, - text_elements: Vec::new(), - }], + input: build_v2_input(user_message, skill_selection), ..Default::default() })?; println!("< turn/start response: {turn_response:?}"); @@ -679,6 +740,7 @@ fn trigger_cmd_approval( config_overrides: &[String], user_message: Option, dynamic_tools: &Option>, + skill_selection: Option<&SkillSelection>, ) -> Result<()> { let default_prompt = "Run `touch /tmp/should-trigger-approval` so I can confirm the file exists."; @@ -692,6 +754,7 @@ fn trigger_cmd_approval( access: ReadOnlyAccess::FullAccess, }), dynamic_tools, + skill_selection, ) } @@ -700,6 +763,7 @@ fn trigger_patch_approval( config_overrides: &[String], user_message: Option, dynamic_tools: &Option>, + skill_selection: Option<&SkillSelection>, ) -> Result<()> { let default_prompt = "Create a file named APPROVAL_DEMO.txt containing a short hello message using apply_patch."; @@ -713,6 +777,7 @@ fn trigger_patch_approval( access: ReadOnlyAccess::FullAccess, }), dynamic_tools, + skill_selection, ) } @@ -720,6 +785,7 @@ fn no_trigger_cmd_approval( endpoint: &Endpoint, config_overrides: &[String], dynamic_tools: &Option>, + skill_selection: Option<&SkillSelection>, ) -> Result<()> { let prompt = "Run `touch should_not_trigger_approval.txt`"; send_message_v2_with_policies( @@ -729,6 +795,7 @@ fn no_trigger_cmd_approval( None, None, dynamic_tools, + skill_selection, ) } @@ -739,6 +806,7 @@ fn send_message_v2_with_policies( approval_policy: Option, sandbox_policy: Option, dynamic_tools: &Option>, + skill_selection: Option<&SkillSelection>, ) -> Result<()> { let mut client = CodexClient::connect(endpoint, config_overrides)?; @@ -752,11 +820,7 @@ fn send_message_v2_with_policies( println!("< thread/start response: {thread_response:?}"); let mut turn_params = TurnStartParams { thread_id: thread_response.thread.id.clone(), - input: vec![V2UserInput::Text { - text: user_message, - // Test client sends plain text without UI element ranges. - text_elements: Vec::new(), - }], + input: build_v2_input(user_message, skill_selection), ..Default::default() }; turn_params.approval_policy = approval_policy; @@ -776,6 +840,7 @@ fn send_follow_up_v2( first_message: String, follow_up_message: String, dynamic_tools: &Option>, + skill_selection: Option<&SkillSelection>, ) -> Result<()> { let mut client = CodexClient::connect(endpoint, config_overrides)?; @@ -790,11 +855,7 @@ fn send_follow_up_v2( let first_turn_params = TurnStartParams { thread_id: thread_response.thread.id.clone(), - input: vec![V2UserInput::Text { - text: first_message, - // Test client sends plain text without UI element ranges. - text_elements: Vec::new(), - }], + input: build_v2_input(first_message, skill_selection), ..Default::default() }; let first_turn_response = client.turn_start(first_turn_params)?; @@ -803,11 +864,7 @@ fn send_follow_up_v2( let follow_up_params = TurnStartParams { thread_id: thread_response.thread.id.clone(), - input: vec![V2UserInput::Text { - text: follow_up_message, - // Test client sends plain text without UI element ranges. - text_elements: Vec::new(), - }], + input: build_v2_input(follow_up_message, skill_selection), ..Default::default() }; let follow_up_response = client.turn_start(follow_up_params)?; @@ -903,6 +960,47 @@ fn ensure_dynamic_tools_unused( Ok(()) } +fn ensure_skill_unused(skill_selection: &Option, command: &str) -> Result<()> { + if skill_selection.is_some() { + bail!( + "skill input is only supported for v2 turn/start commands; remove --skill-name/--skill-path for {command}" + ); + } + Ok(()) +} + +fn parse_skill_selection( + skill_name: Option, + skill_path: Option, +) -> Result> { + match (skill_name, skill_path) { + (None, None) => Ok(None), + (Some(name), Some(path)) => Ok(Some(SkillSelection { name, path })), + (Some(_), None) => bail!("--skill-name requires --skill-path"), + (None, Some(_)) => bail!("--skill-path requires --skill-name"), + } +} + +fn build_v2_input( + user_message: String, + skill_selection: Option<&SkillSelection>, +) -> Vec { + let mut input = vec![V2UserInput::Text { + text: user_message, + // Test client sends plain text without UI element ranges. + text_elements: Vec::new(), + }]; + + if let Some(skill_selection) = skill_selection { + input.push(V2UserInput::Skill { + name: skill_selection.name.clone(), + path: skill_selection.path.clone(), + }); + } + + input +} + fn parse_dynamic_tools_arg(dynamic_tools: &Option) -> Result>> { let Some(raw_arg) = dynamic_tools.as_deref() else { return Ok(None); @@ -949,6 +1047,7 @@ struct CodexClient { #[derive(Debug, Clone, Copy)] enum CommandApprovalBehavior { + Prompt, AlwaysAccept, AbortOn(usize), } @@ -999,7 +1098,7 @@ impl CodexClient { stdout: BufReader::new(stdout), }, pending_notifications: VecDeque::new(), - command_approval_behavior: CommandApprovalBehavior::AlwaysAccept, + command_approval_behavior: CommandApprovalBehavior::Prompt, command_approval_count: 0, command_approval_item_ids: Vec::new(), command_execution_statuses: Vec::new(), @@ -1020,7 +1119,7 @@ impl CodexClient { socket: Box::new(socket), }, pending_notifications: VecDeque::new(), - command_approval_behavior: CommandApprovalBehavior::AlwaysAccept, + command_approval_behavior: CommandApprovalBehavior::Prompt, command_approval_count: 0, command_approval_item_ids: Vec::new(), command_execution_statuses: Vec::new(), @@ -1522,19 +1621,20 @@ impl CodexClient { } let decision = match self.command_approval_behavior { + CommandApprovalBehavior::Prompt => { + self.command_approval_decision(proposed_execpolicy_amendment)? + } CommandApprovalBehavior::AlwaysAccept => CommandExecutionApprovalDecision::Accept, CommandApprovalBehavior::AbortOn(index) if self.command_approval_count == index => { CommandExecutionApprovalDecision::Cancel } CommandApprovalBehavior::AbortOn(_) => CommandExecutionApprovalDecision::Accept, }; - let response = CommandExecutionRequestApprovalResponse { - decision: decision.clone(), - }; + let response = CommandExecutionRequestApprovalResponse { decision }; self.send_server_request_response(request_id, &response)?; println!( "< commandExecution decision for approval #{} on item {item_id}: {:?}", - self.command_approval_count, decision + self.command_approval_count, response.decision ); Ok(()) } @@ -1562,14 +1662,39 @@ impl CodexClient { println!("< grant root: {}", grant_root.display()); } - let response = FileChangeRequestApprovalResponse { - decision: FileChangeApprovalDecision::Accept, - }; + let decision = self.file_change_approval_decision()?; + let response = FileChangeRequestApprovalResponse { decision }; self.send_server_request_response(request_id, &response)?; - println!("< approved fileChange request for item {item_id}"); + println!( + "< responded to fileChange request for item {item_id}: {:?}", + response.decision + ); Ok(()) } + fn command_approval_decision( + &self, + proposed_execpolicy_amendment: Option, + ) -> Result { + if let Some(execpolicy_amendment) = proposed_execpolicy_amendment { + return prompt_for_command_approval_with_amendment(execpolicy_amendment); + } + + if prompt_for_yes_no("Approve command execution request? [y/n] ")? { + Ok(CommandExecutionApprovalDecision::Accept) + } else { + Ok(CommandExecutionApprovalDecision::Decline) + } + } + + fn file_change_approval_decision(&self) -> Result { + if prompt_for_yes_no("Approve file-change request? [y/n] ")? { + Ok(FileChangeApprovalDecision::Accept) + } else { + Ok(FileChangeApprovalDecision::Decline) + } + } + fn send_server_request_response(&mut self, request_id: RequestId, response: &T) -> Result<()> where T: Serialize, @@ -1644,6 +1769,59 @@ fn print_multiline_with_prefix(prefix: &str, payload: &str) { } } +fn prompt_for_yes_no(prompt: &str) -> Result { + loop { + print!("{prompt}"); + io::stdout() + .flush() + .context("failed to flush approval prompt")?; + + let mut line = String::new(); + io::stdin() + .read_line(&mut line) + .context("failed to read approval input")?; + let input = line.trim().to_ascii_lowercase(); + if matches!(input.as_str(), "y" | "yes") { + return Ok(true); + } + if matches!(input.as_str(), "n" | "no") { + return Ok(false); + } + println!("please answer y or n"); + } +} + +fn prompt_for_command_approval_with_amendment( + execpolicy_amendment: ExecPolicyAmendment, +) -> Result { + loop { + print!("Approve command execution request? [y/n/a] (a=always allow) "); + io::stdout() + .flush() + .context("failed to flush approval prompt")?; + + let mut line = String::new(); + io::stdin() + .read_line(&mut line) + .context("failed to read approval input")?; + let input = line.trim().to_ascii_lowercase(); + if matches!(input.as_str(), "y" | "yes") { + return Ok(CommandExecutionApprovalDecision::Accept); + } + if matches!(input.as_str(), "n" | "no") { + return Ok(CommandExecutionApprovalDecision::Decline); + } + if matches!(input.as_str(), "a" | "always" | "always allow") { + return Ok( + CommandExecutionApprovalDecision::AcceptWithExecpolicyAmendment { + execpolicy_amendment: execpolicy_amendment.clone(), + }, + ); + } + println!("please answer y, n, or a"); + } +} + impl Drop for CodexClient { fn drop(&mut self) { let ClientTransport::Stdio { child, stdin, .. } = &mut self.transport else { diff --git a/codex-rs/core/src/exec_policy.rs b/codex-rs/core/src/exec_policy.rs index 62ebdde43d..9644d615a2 100644 --- a/codex-rs/core/src/exec_policy.rs +++ b/codex-rs/core/src/exec_policy.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::io::ErrorKind; use std::path::Path; use std::path::PathBuf; @@ -161,6 +162,15 @@ impl ExecPolicyManager { pub(crate) async fn create_exec_approval_requirement_for_command( &self, req: ExecApprovalRequest<'_>, + ) -> ExecApprovalRequirement { + self.create_exec_approval_requirement_for_command_with_overlay(req, &[]) + .await + } + + pub(crate) async fn create_exec_approval_requirement_for_command_with_overlay( + &self, + req: ExecApprovalRequest<'_>, + overlay_allow_prefixes: &[Vec], ) -> ExecApprovalRequirement { let ExecApprovalRequest { command, @@ -170,6 +180,12 @@ impl ExecPolicyManager { prefix_rule, } = req; let exec_policy = self.current(); + let exec_policy = with_overlay_allow_prefixes(exec_policy.as_ref(), overlay_allow_prefixes); + let overlay_allow_prefixes = overlay_allow_prefixes + .iter() + .filter(|prefix| !prefix.is_empty()) + .cloned() + .collect::>(); let (commands, used_complex_parsing) = commands_for_exec_policy(command); // Keep heredoc prefix parsing for rule evaluation so existing // allow/prompt/forbidden rules still apply, but avoid auto-derived @@ -218,7 +234,9 @@ impl ExecPolicyManager { Decision::Allow => ExecApprovalRequirement::Skip { // Bypass sandbox if execpolicy allows the command bypass_sandbox: evaluation.matched_rules.iter().any(|rule_match| { - is_policy_match(rule_match) && rule_match.decision() == Decision::Allow + is_policy_match(rule_match) + && rule_match.decision() == Decision::Allow + && !is_overlay_allow_rule_match(rule_match, &overlay_allow_prefixes) }), proposed_execpolicy_amendment: if auto_amendment_allowed { try_derive_execpolicy_amendment_for_allow_rules(&evaluation.matched_rules) @@ -255,6 +273,37 @@ impl ExecPolicyManager { } } +fn with_overlay_allow_prefixes(policy: &Policy, overlay_allow_prefixes: &[Vec]) -> Policy { + if overlay_allow_prefixes.is_empty() { + return policy.clone(); + } + + let mut merged = policy.clone(); + for prefix in overlay_allow_prefixes { + if prefix.is_empty() { + continue; + } + if let Err(err) = merged.add_prefix_rule(prefix, Decision::Allow) { + tracing::warn!("failed to append in-memory skill prefix rule {prefix:?}: {err}"); + } + } + merged +} + +fn is_overlay_allow_rule_match( + rule_match: &RuleMatch, + overlay_allow_prefixes: &HashSet>, +) -> bool { + match rule_match { + RuleMatch::PrefixRuleMatch { + matched_prefix, + decision: Decision::Allow, + .. + } => overlay_allow_prefixes.contains(matched_prefix), + RuleMatch::PrefixRuleMatch { .. } | RuleMatch::HeuristicsRuleMatch { .. } => false, + } +} + impl Default for ExecPolicyManager { fn default() -> Self { Self::new(Arc::new(Policy::empty())) @@ -1712,6 +1761,32 @@ prefix_rule( ); } + #[tokio::test] + async fn in_memory_skill_overlay_allow_rule_does_not_bypass_sandbox() { + let command = vec_str(&["skills/demo/scripts/run.sh"]); + let manager = ExecPolicyManager::default(); + let requirement = manager + .create_exec_approval_requirement_for_command_with_overlay( + ExecApprovalRequest { + command: &command, + approval_policy: AskForApproval::OnRequest, + sandbox_policy: &SandboxPolicy::new_read_only_policy(), + sandbox_permissions: SandboxPermissions::UseDefault, + prefix_rule: None, + }, + std::slice::from_ref(&command), + ) + .await; + + assert_eq!( + requirement, + ExecApprovalRequirement::Skip { + bypass_sandbox: false, + proposed_execpolicy_amendment: None, + } + ); + } + fn vec_str(items: &[&str]) -> Vec { items.iter().map(std::string::ToString::to_string).collect() } diff --git a/codex-rs/core/src/skills/command_policy.rs b/codex-rs/core/src/skills/command_policy.rs new file mode 100644 index 0000000000..5a8feceb6b --- /dev/null +++ b/codex-rs/core/src/skills/command_policy.rs @@ -0,0 +1,445 @@ +use std::collections::HashSet; +use std::path::Component; +use std::path::Path; +use std::path::PathBuf; + +use codex_protocol::parse_command::ParsedCommand; +use dunce::canonicalize as canonicalize_path; + +use crate::config::Permissions; +use crate::path_utils::normalize_for_path_comparison; +use crate::skills::SkillLoadOutcome; + +/// Derives transient in-memory execpolicy prefix rules for commands executed +/// from skill `scripts/` directories. +/// +/// Assumptions: +/// 1. Skill prefix derivation is enabled only when `shell_zsh_fork` is enabled. +/// 2. `command` contains the executable and arguments for the invocation. +/// 3. `command_cwd` reflects the effective command target location. +/// 4. Only commands executed via `zsh` are eligible. +/// 5. Only executable paths under `/scripts` are eligible. +/// +/// Return shape: +/// - Outer `Vec`: all derived prefix rules. +/// - Inner `Vec`: one prefix rule pattern as command tokens. +/// +/// Returns an empty list when `shell_zsh_fork` is disabled, command shell is +/// not `zsh`, or when no enabled skill with permissions matches an eligible +/// command action executable. +pub(crate) fn derive_skill_execpolicy_overlay_prefixes_for_command( + skills_outcome: &SkillLoadOutcome, + shell_zsh_fork_enabled: bool, + command: &[String], + command_cwd: &Path, + command_actions: &[ParsedCommand], +) -> Vec> { + if !shell_zsh_fork_enabled { + return Vec::new(); + } + + let command_executable_name = command + .first() + .and_then(|program| Path::new(program).file_name()) + .and_then(|name| name.to_str()); + if command_executable_name != Some("zsh") { + return Vec::new(); + } + + let mut prefixes = Vec::new(); + let mut seen = HashSet::new(); + for command_action in command_actions { + let Some((action_candidate, executable_prefix_token)) = + command_action_candidate(command_action, command_cwd) + else { + continue; + }; + if match_skill_for_candidate(skills_outcome, &action_candidate).is_none() { + continue; + } + let prefix = vec![executable_prefix_token]; + if seen.insert(prefix.clone()) { + prefixes.push(prefix); + } + } + + prefixes +} + +fn command_action_candidate( + command_action: &ParsedCommand, + command_cwd: &Path, +) -> Option<(PathBuf, String)> { + let (action_path, executable_prefix_token) = match command_action { + ParsedCommand::Unknown { cmd } => { + let tokens = shlex::split(cmd)?; + let executable = tokens.first()?; + let executable_path = PathBuf::from(executable); + if !executable_path.is_absolute() && !executable.contains('/') { + return None; + } + Some((executable_path, executable.clone())) + } + ParsedCommand::Read { .. } + | ParsedCommand::ListFiles { .. } + | ParsedCommand::Search { .. } => None, + }?; + let action_path = if action_path.is_absolute() { + action_path + } else { + command_cwd.join(action_path) + }; + let normalized_action_path = normalize_candidate_path(action_path.as_path())?; + Some((normalized_action_path, executable_prefix_token)) +} + +fn normalize_candidate_path(path: &Path) -> Option { + let normalized = normalize_lexically(path); + let canonicalized = canonicalize_path(&normalized).unwrap_or(normalized); + let comparison_path = + normalize_for_path_comparison(canonicalized.as_path()).unwrap_or(canonicalized); + if comparison_path.is_absolute() { + Some(comparison_path) + } else { + None + } +} + +fn match_skill_for_candidate<'a>( + skills_outcome: &'a SkillLoadOutcome, + candidate: &Path, +) -> Option<&'a Permissions> { + for skill in &skills_outcome.skills { + // Disabled skills must not contribute sandbox policy extensions. + if skills_outcome.disabled_paths.contains(&skill.path) { + continue; + } + // Skills without a permissions block cannot supply sandbox policy. + let Some(permissions) = skill.permissions.as_ref() else { + continue; + }; + // Match against the containing skill directory, not the SKILL.md file. + let Some(skill_dir) = skill.path.parent() else { + continue; + }; + let skill_scripts_dir = skill_dir.join("scripts"); + // Normalize before comparing so path containment checks are stable. + let Some(skill_scripts_dir) = normalize_candidate_path(&skill_scripts_dir) else { + continue; + }; + // The executable must live inside the skill's scripts directory. + if !candidate.starts_with(&skill_scripts_dir) { + continue; + } + return Some(permissions); + } + + None +} + +fn normalize_lexically(path: &Path) -> PathBuf { + let mut normalized = PathBuf::new(); + for component in path.components() { + match component { + Component::CurDir => {} + Component::ParentDir => { + normalized.pop(); + } + Component::RootDir | Component::Prefix(_) | Component::Normal(_) => { + normalized.push(component.as_os_str()); + } + } + } + normalized +} + +#[cfg(test)] +mod tests { + use super::derive_skill_execpolicy_overlay_prefixes_for_command; + use crate::config::Constrained; + use crate::config::Permissions; + use crate::config::types::ShellEnvironmentPolicy; + use crate::protocol::AskForApproval; + use crate::protocol::SandboxPolicy; + use crate::skills::SkillLoadOutcome; + use crate::skills::model::SkillMetadata; + use codex_protocol::parse_command::ParsedCommand; + use codex_protocol::protocol::SkillScope; + use pretty_assertions::assert_eq; + use std::collections::HashSet; + use std::path::Path; + use std::path::PathBuf; + + fn skill_with_permissions(skill_path: PathBuf) -> SkillMetadata { + SkillMetadata { + name: "skill".to_string(), + description: "skill".to_string(), + short_description: None, + interface: None, + dependencies: None, + policy: None, + permissions: Some(Permissions { + approval_policy: Constrained::allow_any(AskForApproval::Never), + sandbox_policy: Constrained::allow_any(SandboxPolicy::new_workspace_write_policy()), + network: None, + shell_environment_policy: ShellEnvironmentPolicy::default(), + windows_sandbox_mode: None, + macos_seatbelt_profile_extensions: None, + }), + path: skill_path, + scope: SkillScope::User, + } + } + + fn outcome_with_skills(skills: Vec) -> SkillLoadOutcome { + SkillLoadOutcome { + skills, + errors: Vec::new(), + disabled_paths: HashSet::new(), + } + } + + fn canonical(path: &Path) -> PathBuf { + dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) + } + + #[test] + fn derives_prefix_for_zsh_executable_inside_skill_scripts_directory() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_dir = tempdir.path().join("skills/demo"); + let scripts_dir = skill_dir.join("scripts"); + std::fs::create_dir_all(&scripts_dir).expect("create scripts"); + std::fs::write(scripts_dir.join("run.sh"), "#!/bin/sh\necho ok\n").expect("write script"); + let skill_path = skill_dir.join("SKILL.md"); + std::fs::write(&skill_path, "skill").expect("write SKILL.md"); + let cwd = tempdir.path().to_path_buf(); + + let outcome = outcome_with_skills(vec![skill_with_permissions(canonical(&skill_path))]); + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "skills/demo/scripts/run.sh".to_string(), + ]; + let command_actions = vec![ParsedCommand::Unknown { + cmd: "skills/demo/scripts/run.sh".to_string(), + }]; + + let resolved = derive_skill_execpolicy_overlay_prefixes_for_command( + &outcome, + true, + &command, + &cwd, + &command_actions, + ); + + assert_eq!( + resolved, + vec![vec!["skills/demo/scripts/run.sh".to_string()]] + ); + } + + #[test] + fn returns_empty_prefixes_when_command_is_not_zsh() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_dir = tempdir.path().join("skills/demo"); + let scripts_dir = skill_dir.join("scripts"); + std::fs::create_dir_all(&scripts_dir).expect("create scripts"); + std::fs::write(scripts_dir.join("run.sh"), "#!/bin/sh\necho ok\n").expect("write script"); + let skill_path = skill_dir.join("SKILL.md"); + std::fs::write(&skill_path, "skill").expect("write SKILL.md"); + + let outcome = outcome_with_skills(vec![skill_with_permissions(canonical(&skill_path))]); + let command = vec![ + "/bin/bash".to_string(), + "-lc".to_string(), + "skills/demo/scripts/run.sh".to_string(), + ]; + let command_actions = vec![ParsedCommand::Unknown { + cmd: "skills/demo/scripts/run.sh".to_string(), + }]; + + let resolved = derive_skill_execpolicy_overlay_prefixes_for_command( + &outcome, + true, + &command, + tempdir.path(), + &command_actions, + ); + + assert!(resolved.is_empty()); + } + + #[test] + fn returns_empty_prefixes_for_paths_outside_skill_scripts_directory() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_dir = tempdir.path().join("skills/demo"); + std::fs::create_dir_all(&skill_dir).expect("create skill"); + std::fs::write(skill_dir.join("run.sh"), "#!/bin/sh\necho ok\n").expect("write script"); + let skill_path = skill_dir.join("SKILL.md"); + std::fs::write(&skill_path, "skill").expect("write SKILL.md"); + + let outcome = outcome_with_skills(vec![skill_with_permissions(canonical(&skill_path))]); + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "skills/demo/run.sh".to_string(), + ]; + let command_actions = vec![ParsedCommand::Unknown { + cmd: "skills/demo/run.sh".to_string(), + }]; + let resolved = derive_skill_execpolicy_overlay_prefixes_for_command( + &outcome, + true, + &command, + tempdir.path(), + &command_actions, + ); + + assert!(resolved.is_empty()); + } + + #[test] + fn ignores_disabled_skill_when_deriving_prefix_rules() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_dir = tempdir.path().join("skills/demo"); + let scripts_dir = skill_dir.join("scripts"); + std::fs::create_dir_all(&scripts_dir).expect("create skill dir"); + std::fs::write(scripts_dir.join("tool.sh"), "#!/bin/sh\necho ok\n").expect("write script"); + let skill_path = skill_dir.join("SKILL.md"); + std::fs::write(&skill_path, "skill").expect("write SKILL.md"); + let skill_path = canonical(&skill_path); + + let mut outcome = outcome_with_skills(vec![skill_with_permissions(skill_path.clone())]); + outcome.disabled_paths.insert(skill_path); + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "skills/demo/scripts/tool.sh".to_string(), + ]; + let command_actions = vec![ParsedCommand::Unknown { + cmd: "skills/demo/scripts/tool.sh".to_string(), + }]; + + let resolved = derive_skill_execpolicy_overlay_prefixes_for_command( + &outcome, + true, + &command, + tempdir.path(), + &command_actions, + ); + + assert!(resolved.is_empty()); + } + + #[test] + fn derives_prefix_for_nested_skill_scripts_path() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let parent_skill_dir = tempdir.path().join("skills/parent"); + let nested_skill_dir = parent_skill_dir.join("nested"); + std::fs::create_dir_all(parent_skill_dir.join("scripts")).expect("create parent scripts"); + std::fs::create_dir_all(nested_skill_dir.join("scripts")).expect("create nested scripts"); + std::fs::write( + parent_skill_dir.join("scripts/run.sh"), + "#!/bin/sh\necho parent\n", + ) + .expect("write script"); + + std::fs::write( + nested_skill_dir.join("scripts/run.sh"), + "#!/bin/sh\necho nested\n", + ) + .expect("write script"); + + let parent_skill_path = parent_skill_dir.join("SKILL.md"); + let nested_skill_path = nested_skill_dir.join("SKILL.md"); + std::fs::write(&parent_skill_path, "parent").expect("write parent skill"); + std::fs::write(&nested_skill_path, "nested").expect("write nested skill"); + + let outcome = outcome_with_skills(vec![ + skill_with_permissions(canonical(&parent_skill_path)), + skill_with_permissions(canonical(&nested_skill_path)), + ]); + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "skills/parent/nested/scripts/run.sh".to_string(), + ]; + let command_actions = vec![ParsedCommand::Unknown { + cmd: "skills/parent/nested/scripts/run.sh".to_string(), + }]; + + let resolved = derive_skill_execpolicy_overlay_prefixes_for_command( + &outcome, + true, + &command, + tempdir.path(), + &command_actions, + ); + + assert_eq!( + resolved, + vec![vec!["skills/parent/nested/scripts/run.sh".to_string()]] + ); + } + + #[test] + fn ignores_non_path_unknown_command_actions() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_dir = tempdir.path().join("skills/demo"); + std::fs::create_dir_all(skill_dir.join("scripts")).expect("create scripts"); + let skill_path = skill_dir.join("SKILL.md"); + std::fs::write(&skill_path, "skill").expect("write SKILL.md"); + + let outcome = outcome_with_skills(vec![skill_with_permissions(canonical(&skill_path))]); + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "echo hi".to_string(), + ]; + let command_actions = vec![ParsedCommand::Unknown { + cmd: "echo hi".to_string(), + }]; + + let resolved = derive_skill_execpolicy_overlay_prefixes_for_command( + &outcome, + true, + &command, + skill_dir.join("scripts").as_path(), + &command_actions, + ); + + assert!(resolved.is_empty()); + } + + #[test] + fn returns_empty_prefixes_when_shell_zsh_fork_is_disabled() { + let tempdir = tempfile::tempdir().expect("tempdir"); + let skill_dir = tempdir.path().join("skills/demo"); + let scripts_dir = skill_dir.join("scripts"); + std::fs::create_dir_all(&scripts_dir).expect("create scripts"); + std::fs::write(scripts_dir.join("run.sh"), "#!/bin/sh\necho ok\n").expect("write script"); + let skill_path = skill_dir.join("SKILL.md"); + std::fs::write(&skill_path, "skill").expect("write SKILL.md"); + let cwd = tempdir.path().to_path_buf(); + + let outcome = outcome_with_skills(vec![skill_with_permissions(canonical(&skill_path))]); + let command = vec![ + "/bin/zsh".to_string(), + "-lc".to_string(), + "skills/demo/scripts/run.sh".to_string(), + ]; + let command_actions = vec![ParsedCommand::Unknown { + cmd: "skills/demo/scripts/run.sh".to_string(), + }]; + + let resolved = derive_skill_execpolicy_overlay_prefixes_for_command( + &outcome, + false, + &command, + &cwd, + &command_actions, + ); + + assert!(resolved.is_empty()); + } +} diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs index 18f7180f99..2620dd0b67 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -1,3 +1,4 @@ +mod command_policy; mod env_var_dependencies; pub mod injection; pub mod loader; @@ -8,6 +9,7 @@ pub mod remote; pub mod render; pub mod system; +pub(crate) use command_policy::derive_skill_execpolicy_overlay_prefixes_for_command; pub(crate) use env_var_dependencies::collect_env_var_dependencies; pub(crate) use env_var_dependencies::resolve_skill_dependencies_for_turn; pub(crate) use injection::SkillInjections; diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 31bf3f1294..d92589f533 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -9,10 +9,13 @@ use crate::codex::TurnContext; use crate::exec::ExecParams; use crate::exec_env::create_env; use crate::exec_policy::ExecApprovalRequest; +use crate::features::Feature; use crate::function_tool::FunctionCallError; use crate::is_safe_command::is_known_safe_command; +use crate::parse_command::parse_command; use crate::protocol::ExecCommandSource; use crate::shell::Shell; +use crate::skills::derive_skill_execpolicy_overlay_prefixes_for_command; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; use crate::tools::context::ToolPayload; @@ -297,16 +300,35 @@ impl ShellHandler { let event_ctx = ToolEventCtx::new(session.as_ref(), turn.as_ref(), &call_id, None); emitter.begin(event_ctx).await; + let skills_outcome = session + .services + .skills_manager + .skills_for_cwd(&turn.cwd, false) + .await; + let shell_zsh_fork_enabled = session.features().enabled(Feature::ShellZshFork); + let command_actions = parse_command(&exec_params.command); + let skill_execpolicy_overlay_prefixes = + derive_skill_execpolicy_overlay_prefixes_for_command( + &skills_outcome, + shell_zsh_fork_enabled, + &exec_params.command, + &exec_params.cwd, + &command_actions, + ); + let exec_approval_requirement = session .services .exec_policy - .create_exec_approval_requirement_for_command(ExecApprovalRequest { - command: &exec_params.command, - approval_policy: turn.approval_policy, - sandbox_policy: &turn.sandbox_policy, - sandbox_permissions: exec_params.sandbox_permissions, - prefix_rule, - }) + .create_exec_approval_requirement_for_command_with_overlay( + ExecApprovalRequest { + command: &exec_params.command, + approval_policy: turn.approval_policy, + sandbox_policy: &turn.sandbox_policy, + sandbox_permissions: exec_params.sandbox_permissions, + prefix_rule, + }, + &skill_execpolicy_overlay_prefixes, + ) .await; let req = ShellRequest {