From c33c2a650ff0cf1cd89ab8c7b3f87ec9fe42ed82 Mon Sep 17 00:00:00 2001 From: celia-oai Date: Tue, 17 Feb 2026 15:46:49 -0800 Subject: [PATCH] changes --- codex-rs/core/src/skills/command_policy.rs | 195 ++++-------------- codex-rs/core/src/tools/handlers/shell.rs | 19 +- .../core/src/unified_exec/process_manager.rs | 13 +- 3 files changed, 54 insertions(+), 173 deletions(-) diff --git a/codex-rs/core/src/skills/command_policy.rs b/codex-rs/core/src/skills/command_policy.rs index 7a70868aa7..abf1b548f7 100644 --- a/codex-rs/core/src/skills/command_policy.rs +++ b/codex-rs/core/src/skills/command_policy.rs @@ -1,109 +1,32 @@ -use std::collections::HashSet; use std::path::Component; use std::path::Path; use std::path::PathBuf; use codex_protocol::protocol::SandboxPolicy; -use dirs::home_dir; use dunce::canonicalize as canonicalize_path; -use crate::bash::parse_shell_lc_plain_commands; -use crate::bash::parse_shell_lc_single_command_prefix; use crate::config::Permissions; use crate::path_utils::normalize_for_path_comparison; use crate::skills::SkillLoadOutcome; -use crate::skills::SkillMetadata; +/// Resolves the sandbox policy extension contributed by the first matching +/// skill for a command invocation. +/// +/// Assumptions: +/// 1. `command_cwd` reflects the effective command target location. +/// 2. If `command_cwd` is contained by multiple skill directories, the first +/// enabled skill in `skills_outcome.skills` wins. +/// 3. Command tokens are not used for matching. +/// +/// Returns `None` when no enabled skill with permissions matches +/// `command_cwd`. pub(crate) fn resolve_skill_sandbox_extension_for_command( skills_outcome: &SkillLoadOutcome, - command: &[String], command_cwd: &Path, ) -> Option { - let segments = command_segments_for_matching(command); - let mut best_match: Option<(usize, SandboxPolicy)> = None; - - for segment in segments { - for candidate in candidate_paths_for_segment(&segment, command_cwd) { - let Some((depth, _, permissions)) = - match_skill_for_candidate(skills_outcome, candidate.as_path()) - else { - continue; - }; - - let should_replace = match &best_match { - Some((best_depth, _)) => depth > *best_depth, - None => true, - }; - if should_replace { - best_match = Some((depth, permissions.sandbox_policy.get().clone())); - } - } - } - - best_match.map(|(_, sandbox_policy)| sandbox_policy) -} - -fn command_segments_for_matching(command: &[String]) -> Vec> { - if let Some(commands) = parse_shell_lc_plain_commands(command) - && !commands.is_empty() - { - return commands; - } - - if let Some(command) = parse_shell_lc_single_command_prefix(command) { - return vec![command]; - } - - vec![command.to_vec()] -} - -fn candidate_paths_for_segment(segment: &[String], command_cwd: &Path) -> Vec { - let mut candidates = Vec::new(); - let mut seen = HashSet::new(); - - if let Some(path) = normalize_candidate_path(command_cwd) - && seen.insert(path.clone()) - { - candidates.push(path); - } - - for token in segment { - let Some(path) = candidate_path_from_token(token, command_cwd) else { - continue; - }; - if seen.insert(path.clone()) { - candidates.push(path); - } - } - - candidates -} - -fn candidate_path_from_token(token: &str, command_cwd: &Path) -> Option { - if token.is_empty() || token.contains("://") || token.starts_with('-') { - return None; - } - - let is_path_like = token == "~" - || token.starts_with("~/") - || token.starts_with("./") - || token.starts_with("../") - || token.contains('/') - || token.contains('\\') - || Path::new(token).is_absolute() - || command_cwd.join(token).exists(); - if !is_path_like { - return None; - } - - let expanded = expand_home(token); - let path = PathBuf::from(expanded); - let absolute = if path.is_absolute() { - path - } else { - command_cwd.join(path) - }; - normalize_candidate_path(&absolute) + let candidate = normalize_candidate_path(command_cwd)?; + let permissions = match_skill_for_candidate(skills_outcome, candidate.as_path())?; + Some(permissions.sandbox_policy.get().clone()) } fn normalize_candidate_path(path: &Path) -> Option { @@ -121,52 +44,32 @@ fn normalize_candidate_path(path: &Path) -> Option { fn match_skill_for_candidate<'a>( skills_outcome: &'a SkillLoadOutcome, candidate: &Path, -) -> Option<(usize, &'a SkillMetadata, &'a Permissions)> { - let mut best_match: Option<(usize, &SkillMetadata, &Permissions)> = None; - +) -> 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; }; + // Normalize before comparing so path containment checks are stable. let Some(skill_dir) = normalize_candidate_path(skill_dir) else { continue; }; + // The command cwd must be inside the skill directory. if !candidate.starts_with(&skill_dir) { continue; } - - let depth = skill_dir.components().count(); - let should_replace = match &best_match { - Some((best_depth, _, _)) => depth > *best_depth, - None => true, - }; - if should_replace { - best_match = Some((depth, skill, permissions)); - } + return Some(permissions); } - best_match -} - -fn expand_home(path: &str) -> String { - if path == "~" { - if let Some(home) = home_dir() { - return home.to_string_lossy().to_string(); - } - return path.to_string(); - } - if let Some(rest) = path.strip_prefix("~/") - && let Some(home) = home_dir() - { - return home.join(rest).to_string_lossy().to_string(); - } - path.to_string() + None } fn normalize_lexically(path: &Path) -> PathBuf { @@ -237,13 +140,11 @@ mod tests { } #[test] - fn resolves_skill_policy_for_executable_inside_skill_directory() { + fn resolves_skill_policy_when_cwd_is_inside_skill_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"); - let executable = scripts_dir.join("run.sh"); - std::fs::write(&executable, "#!/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"); @@ -260,19 +161,17 @@ mod tests { skill_policy.clone(), )]); - let resolved = resolve_skill_sandbox_extension_for_command( - &outcome, - &[canonical(&executable).to_string_lossy().to_string()], - tempdir.path(), - ); + let resolved = resolve_skill_sandbox_extension_for_command(&outcome, &scripts_dir); assert_eq!(resolved, Some(skill_policy)); } #[test] - fn resolves_skill_policy_for_shell_wrapped_relative_script_command() { + fn does_not_resolve_policy_when_only_command_path_matches() { let tempdir = tempfile::tempdir().expect("tempdir"); let skill_dir = tempdir.path().join("skills/demo"); + let outside_dir = tempdir.path().join("outside"); + std::fs::create_dir_all(&outside_dir).expect("create outside"); 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"); @@ -292,17 +191,9 @@ mod tests { skill_policy.clone(), )]); - let resolved = resolve_skill_sandbox_extension_for_command( - &outcome, - &[ - "bash".to_string(), - "-lc".to_string(), - "./scripts/run.sh --flag".to_string(), - ], - &skill_dir, - ); + let resolved = resolve_skill_sandbox_extension_for_command(&outcome, &outside_dir); - assert_eq!(resolved, Some(skill_policy)); + assert_eq!(resolved, None); } #[test] @@ -310,8 +201,7 @@ mod tests { let tempdir = tempfile::tempdir().expect("tempdir"); let skill_dir = tempdir.path().join("skills/demo"); std::fs::create_dir_all(&skill_dir).expect("create skill dir"); - let executable = skill_dir.join("tool.sh"); - std::fs::write(&executable, "#!/bin/sh\necho ok\n").expect("write script"); + std::fs::write(skill_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); @@ -322,24 +212,23 @@ mod tests { )]); outcome.disabled_paths.insert(skill_path); - let resolved = resolve_skill_sandbox_extension_for_command( - &outcome, - &[canonical(&executable).to_string_lossy().to_string()], - tempdir.path(), - ); + let resolved = resolve_skill_sandbox_extension_for_command(&outcome, &skill_dir); assert_eq!(resolved, None); } #[test] - fn prefers_most_specific_skill_directory_for_nested_match() { + fn resolves_first_matching_skill_directory_for_nested_match() { 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(nested_skill_dir.join("scripts")).expect("create scripts"); - let executable = nested_skill_dir.join("scripts/run.sh"); - std::fs::write(&executable, "#!/bin/sh\necho ok\n").expect("write script"); + std::fs::write( + nested_skill_dir.join("scripts/run.sh"), + "#!/bin/sh\necho ok\n", + ) + .expect("write script"); let parent_skill_path = parent_skill_dir.join("SKILL.md"); let nested_skill_path = nested_skill_dir.join("SKILL.md"); @@ -364,16 +253,12 @@ mod tests { exclude_slash_tmp: false, }; let outcome = outcome_with_skills(vec![ - skill_with_policy(canonical(&parent_skill_path), parent_policy), + skill_with_policy(canonical(&parent_skill_path), parent_policy.clone()), skill_with_policy(canonical(&nested_skill_path), nested_policy.clone()), ]); - let resolved = resolve_skill_sandbox_extension_for_command( - &outcome, - &[canonical(&executable).to_string_lossy().to_string()], - tempdir.path(), - ); + let resolved = resolve_skill_sandbox_extension_for_command(&outcome, &nested_skill_dir); - assert_eq!(resolved, Some(nested_policy)); + assert_eq!(resolved, Some(parent_policy)); } } diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 7e4e85f4e0..dca6e36ca8 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -304,17 +304,14 @@ impl ShellHandler { .skills_manager .skills_for_cwd(&turn.cwd, false) .await; - let effective_sandbox_policy = resolve_skill_sandbox_extension_for_command( - &skills_outcome, - &exec_params.command, - &exec_params.cwd, - ) - .map_or_else( - || turn.sandbox_policy.clone(), - |skill_sandbox_policy| { - extend_sandbox_policy(&turn.sandbox_policy, &skill_sandbox_policy) - }, - ); + let effective_sandbox_policy = + resolve_skill_sandbox_extension_for_command(&skills_outcome, &exec_params.cwd) + .map_or_else( + || turn.sandbox_policy.clone(), + |skill_sandbox_policy| { + extend_sandbox_policy(&turn.sandbox_policy, &skill_sandbox_policy) + }, + ); let exec_approval_requirement = session .services diff --git a/codex-rs/core/src/unified_exec/process_manager.rs b/codex-rs/core/src/unified_exec/process_manager.rs index 77ee5be632..6c6ff032b2 100644 --- a/codex-rs/core/src/unified_exec/process_manager.rs +++ b/codex-rs/core/src/unified_exec/process_manager.rs @@ -615,13 +615,12 @@ impl UnifiedExecProcessManager { .skills_for_cwd(&context.turn.cwd, false) .await; let effective_sandbox_policy = - resolve_skill_sandbox_extension_for_command(&skills_outcome, &request.command, &cwd) - .map_or_else( - || context.turn.sandbox_policy.clone(), - |skill_sandbox_policy| { - extend_sandbox_policy(&context.turn.sandbox_policy, &skill_sandbox_policy) - }, - ); + resolve_skill_sandbox_extension_for_command(&skills_outcome, &cwd).map_or_else( + || context.turn.sandbox_policy.clone(), + |skill_sandbox_policy| { + extend_sandbox_policy(&context.turn.sandbox_policy, &skill_sandbox_policy) + }, + ); let exec_approval_requirement = context .session .services