changes

Author: celia-oai

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<SandboxPolicy> {
-    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<Vec<String>> {
-    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<PathBuf> {
-    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<PathBuf> {
-    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<PathBuf> {
@@ -121,9 +44,7 @@ fn normalize_candidate_path(path: &Path) -> Option<PathBuf> {
 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 {
         if skills_outcome.disabled_paths.contains(&skill.path) {
             continue;
@@ -140,33 +61,10 @@ fn match_skill_for_candidate<'a>(
         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 +135,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 +156,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,26 +186,17 @@ 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]
     fn ignores_disabled_skill_when_resolving_command_policy() {
         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 +207,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 +248,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));
     }
 }

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

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