diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index a1aa3c630e..126d153d87 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -12,11 +12,12 @@ use crate::skills::model::SkillLoadOutcome; use crate::skills::model::SkillMetadata; use crate::skills::model::SkillPolicy; use crate::skills::model::SkillToolDependency; +use crate::skills::permissions::SkillPermissionProfile; use crate::skills::permissions::compile_permission_profile; use crate::skills::system::system_cache_root_dir; use codex_app_server_protocol::ConfigLayerSource; -use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SkillScope; +use codex_utils_absolute_path::AbsolutePathBufGuard; use dirs::home_dir; use dunce::canonicalize as canonicalize_path; use serde::Deserialize; @@ -54,7 +55,7 @@ struct SkillMetadataFile { #[serde(default)] policy: Option, #[serde(default)] - permissions: Option, + permissions: Option, } #[derive(Default)] @@ -62,7 +63,7 @@ struct LoadedSkillMetadata { interface: Option, dependencies: Option, policy: Option, - permission_profile: Option, + permission_profile: Option, permissions: Option, } @@ -573,7 +574,10 @@ fn load_skill_metadata(skill_path: &Path) -> LoadedSkillMetadata { } }; - let parsed: SkillMetadataFile = match serde_yaml::from_str(&contents) { + let parsed: SkillMetadataFile = match { + let _guard = AbsolutePathBufGuard::new(skill_dir); + serde_yaml::from_str(&contents) + } { Ok(parsed) => parsed, Err(error) => { tracing::warn!( @@ -598,7 +602,7 @@ fn load_skill_metadata(skill_path: &Path) -> LoadedSkillMetadata { dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), permission_profile, - permissions: compile_permission_profile(skill_dir, permissions), + permissions: compile_permission_profile(permissions.as_ref()), } } @@ -838,10 +842,10 @@ mod tests { use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigRequirements; use crate::config_loader::ConfigRequirementsToml; + use crate::skills::permissions::SkillFileSystemPermissions; + use crate::skills::permissions::SkillPermissionProfile; use codex_config::CONFIG_TOML_FILE; use codex_protocol::config_types::TrustLevel; - use codex_protocol::models::FileSystemPermissions; - use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; @@ -1373,11 +1377,17 @@ permissions: assert_eq!(outcome.skills.len(), 1); assert_eq!( outcome.skills[0].permission_profile, - Some(PermissionProfile { + Some(SkillPermissionProfile { network: Some(true), - file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("./data")]), - write: Some(vec![PathBuf::from("./output")]), + file_system: Some(SkillFileSystemPermissions { + read: Some(vec![ + AbsolutePathBuf::try_from(normalized(skill_dir.join("data").as_path())) + .expect("absolute data path"), + ]), + write: Some(vec![ + AbsolutePathBuf::try_from(normalized(skill_dir.join("output").as_path())) + .expect("absolute output path"), + ]), }), macos: None, }) diff --git a/codex-rs/core/src/skills/model.rs b/codex-rs/core/src/skills/model.rs index ddb1dd2638..f3432c3631 100644 --- a/codex-rs/core/src/skills/model.rs +++ b/codex-rs/core/src/skills/model.rs @@ -4,7 +4,7 @@ use std::path::PathBuf; use std::sync::Arc; use crate::config::Permissions; -use codex_protocol::models::PermissionProfile; +use crate::skills::permissions::SkillPermissionProfile; use codex_protocol::protocol::SkillScope; #[derive(Debug, Clone, PartialEq)] @@ -15,7 +15,7 @@ pub struct SkillMetadata { pub interface: Option, pub dependencies: Option, pub policy: Option, - pub permission_profile: Option, + pub permission_profile: Option, // This is an experimental field. pub permissions: Option, /// Path to the SKILLS.md file that declares this skill. diff --git a/codex-rs/core/src/skills/permissions.rs b/codex-rs/core/src/skills/permissions.rs index 89c49709e3..3b10833a3d 100644 --- a/codex-rs/core/src/skills/permissions.rs +++ b/codex-rs/core/src/skills/permissions.rs @@ -1,8 +1,6 @@ -use std::collections::HashSet; -use std::path::Component; -use std::path::Path; use std::path::PathBuf; +use codex_protocol::models::FileSystemPermissions; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsAutomationValue; use codex_protocol::models::MacOsPermissions; @@ -11,8 +9,8 @@ use codex_protocol::models::MacOsPreferencesValue; use codex_protocol::models::MacOsSeatbeltProfileExtensions; use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; -use dirs::home_dir; use dunce::canonicalize as canonicalize_path; +use serde::Deserialize; use tracing::warn; use crate::config::Constrained; @@ -22,23 +20,86 @@ use crate::protocol::AskForApproval; use crate::protocol::ReadOnlyAccess; use crate::protocol::SandboxPolicy; +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize)] +pub struct SkillFileSystemPermissions { + pub read: Option>, + pub write: Option>, +} + +impl SkillFileSystemPermissions { + pub(crate) fn is_empty(&self) -> bool { + self.read.is_none() && self.write.is_none() + } +} + +impl From<&SkillFileSystemPermissions> for FileSystemPermissions { + fn from(value: &SkillFileSystemPermissions) -> Self { + Self { + read: value + .read + .as_ref() + .map(|paths| absolute_paths_to_path_bufs(paths)), + write: value + .write + .as_ref() + .map(|paths| absolute_paths_to_path_bufs(paths)), + } + } +} + +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize)] +pub struct SkillPermissionProfile { + pub network: Option, + pub file_system: Option, + pub macos: Option, +} + +impl SkillPermissionProfile { + pub(crate) fn is_empty(&self) -> bool { + self.network.is_none() + && self + .file_system + .as_ref() + .map(SkillFileSystemPermissions::is_empty) + .unwrap_or(true) + && self + .macos + .as_ref() + .map(MacOsPermissions::is_empty) + .unwrap_or(true) + } +} + +impl From<&SkillPermissionProfile> for PermissionProfile { + fn from(value: &SkillPermissionProfile) -> Self { + Self { + network: value.network, + file_system: value.file_system.as_ref().map(FileSystemPermissions::from), + macos: value.macos.clone(), + } + } +} + +impl From for PermissionProfile { + fn from(value: SkillPermissionProfile) -> Self { + Self::from(&value) + } +} + pub(crate) fn compile_permission_profile( - skill_dir: &Path, - permissions: Option, + permissions: Option<&SkillPermissionProfile>, ) -> Option { - let PermissionProfile { - network, - file_system, - macos, - } = permissions?; - let file_system = file_system.unwrap_or_default(); - let fs_read = normalize_permission_paths( - skill_dir, + let permissions = permissions?; + let file_system = permissions + .file_system + .as_ref() + .cloned() + .unwrap_or_default(); + let fs_read = canonicalize_permission_paths( file_system.read.as_deref().unwrap_or_default(), "permissions.file_system.read", ); - let fs_write = normalize_permission_paths( - skill_dir, + let fs_write = canonicalize_permission_paths( file_system.write.as_deref().unwrap_or_default(), "permissions.file_system.write", ); @@ -53,7 +114,7 @@ pub(crate) fn compile_permission_profile( readable_roots: fs_read, } }, - network_access: network.unwrap_or_default(), + network_access: permissions.network.unwrap_or_default(), exclude_tmpdir_env_var: false, exclude_slash_tmp: false, } @@ -68,7 +129,7 @@ pub(crate) fn compile_permission_profile( // Default sandbox policy SandboxPolicy::new_read_only_policy() }; - let macos_permissions = macos.unwrap_or_default(); + let macos_permissions = permissions.macos.clone().unwrap_or_default(); let macos_seatbelt_profile_extensions = build_macos_seatbelt_profile_extensions(&macos_permissions); @@ -83,68 +144,31 @@ pub(crate) fn compile_permission_profile( }) } -fn normalize_permission_paths( - skill_dir: &Path, - values: &[PathBuf], - field: &str, -) -> Vec { - let mut paths = Vec::new(); - let mut seen = HashSet::new(); - - for value in values { - let Some(path) = normalize_permission_path(skill_dir, value, field) else { - continue; - }; - if seen.insert(path.clone()) { - paths.push(path); - } - } - - paths +fn absolute_paths_to_path_bufs(values: &[AbsolutePathBuf]) -> Vec { + values.iter().map(AbsolutePathBuf::to_path_buf).collect() } -fn normalize_permission_path( - skill_dir: &Path, - value: &Path, - field: &str, -) -> Option { - let value = value.to_string_lossy(); - let trimmed = value.trim(); - if trimmed.is_empty() { - warn!("ignoring {field}: value is empty"); - return None; - } - - let expanded = expand_home(trimmed); - let absolute = if expanded.is_absolute() { - expanded - } else { - skill_dir.join(expanded) - }; - let normalized = normalize_lexically(&absolute); - let canonicalized = canonicalize_path(&normalized).unwrap_or(normalized); - match AbsolutePathBuf::from_absolute_path(&canonicalized) { - Ok(path) => Some(path), - Err(error) => { - warn!("ignoring {field}: expected absolute path, got {canonicalized:?}: {error}"); - None - } - } -} - -fn expand_home(path: &str) -> PathBuf { - if path == "~" { - if let Some(home) = home_dir() { - return home; - } - return PathBuf::from(path); - } - if let Some(rest) = path.strip_prefix("~/") - && let Some(home) = home_dir() - { - return home.join(rest); - } - PathBuf::from(path) +fn canonicalize_permission_paths(values: &[AbsolutePathBuf], field: &str) -> Vec { + values + .iter() + .filter_map(|value| { + let canonicalized = canonicalize_path(value.as_path()).unwrap_or(value.to_path_buf()); + match AbsolutePathBuf::from_absolute_path(&canonicalized) { + Ok(path) => Some(path), + Err(error) => { + warn!( + "ignoring {field}: expected absolute path, got {canonicalized:?}: {error}" + ); + None + } + } + }) + .fold(Vec::new(), |mut paths, path| { + if !paths.contains(&path) { + paths.push(path); + } + paths + }) } #[cfg(target_os = "macos")] @@ -233,24 +257,10 @@ fn build_macos_seatbelt_profile_extensions( 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::SkillFileSystemPermissions; + use super::SkillPermissionProfile; use super::compile_permission_profile; use crate::config::Constrained; use crate::config::Permissions; @@ -258,18 +268,15 @@ mod tests { use crate::protocol::AskForApproval; use crate::protocol::ReadOnlyAccess; use crate::protocol::SandboxPolicy; - use codex_protocol::models::FileSystemPermissions; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsAutomationValue; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsPermissions; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsPreferencesValue; - use codex_protocol::models::PermissionProfile; use codex_utils_absolute_path::AbsolutePathBuf; use pretty_assertions::assert_eq; use std::fs; - use std::path::PathBuf; #[test] fn compile_permission_profile_normalizes_paths() { @@ -279,21 +286,22 @@ mod tests { let read_dir = skill_dir.join("data"); fs::create_dir_all(&read_dir).expect("read dir"); - let profile = compile_permission_profile( - &skill_dir, - Some(PermissionProfile { - network: Some(true), - file_system: Some(FileSystemPermissions { - read: Some(vec![ - PathBuf::from("./data"), - PathBuf::from("./data"), - PathBuf::from("scripts/../data"), - ]), - write: Some(vec![PathBuf::from("./output")]), - }), - ..Default::default() + let profile = compile_permission_profile(Some(&SkillPermissionProfile { + network: Some(true), + file_system: Some(SkillFileSystemPermissions { + read: Some(vec![ + AbsolutePathBuf::try_from(skill_dir.join("data")).expect("read path"), + AbsolutePathBuf::try_from(skill_dir.join("data")).expect("read path"), + AbsolutePathBuf::try_from(skill_dir.join("scripts/../data")) + .expect("normalized read path"), + ]), + write: Some(vec![ + AbsolutePathBuf::try_from(skill_dir.join("output")) + .expect("absolute output path"), + ]), }), - ) + ..Default::default() + })) .expect("profile"); assert_eq!( @@ -334,11 +342,7 @@ mod tests { #[test] fn compile_permission_profile_without_permissions_has_empty_profile() { - let tempdir = tempfile::tempdir().expect("tempdir"); - let skill_dir = tempdir.path().join("skill"); - fs::create_dir_all(&skill_dir).expect("skill dir"); - - let profile = compile_permission_profile(&skill_dir, None); + let profile = compile_permission_profile(None); assert_eq!(profile, None); } @@ -349,13 +353,10 @@ mod tests { let skill_dir = tempdir.path().join("skill"); fs::create_dir_all(&skill_dir).expect("skill dir"); - let profile = compile_permission_profile( - &skill_dir, - Some(PermissionProfile { - network: Some(true), - ..Default::default() - }), - ) + let profile = compile_permission_profile(Some(&SkillPermissionProfile { + network: Some(true), + ..Default::default() + })) .expect("profile"); assert_eq!( @@ -384,17 +385,16 @@ mod tests { let read_dir = skill_dir.join("data"); fs::create_dir_all(&read_dir).expect("read dir"); - let profile = compile_permission_profile( - &skill_dir, - Some(PermissionProfile { - network: Some(true), - file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("./data")]), - write: Some(Vec::new()), - }), - ..Default::default() + let profile = compile_permission_profile(Some(&SkillPermissionProfile { + network: Some(true), + file_system: Some(SkillFileSystemPermissions { + read: Some(vec![ + AbsolutePathBuf::try_from(skill_dir.join("data")).expect("absolute read path"), + ]), + write: Some(Vec::new()), }), - ) + ..Default::default() + })) .expect("profile"); assert_eq!( @@ -433,20 +433,17 @@ mod tests { let skill_dir = tempdir.path().join("skill"); fs::create_dir_all(&skill_dir).expect("skill dir"); - let profile = compile_permission_profile( - &skill_dir, - Some(PermissionProfile { - macos: Some(MacOsPermissions { - preferences: Some(MacOsPreferencesValue::Mode("readwrite".to_string())), - automations: Some(MacOsAutomationValue::BundleIds(vec![ - "com.apple.Notes".to_string(), - ])), - accessibility: Some(true), - calendar: Some(true), - }), - ..Default::default() + let profile = compile_permission_profile(Some(&SkillPermissionProfile { + macos: Some(MacOsPermissions { + preferences: Some(MacOsPreferencesValue::Mode("readwrite".to_string())), + automations: Some(MacOsAutomationValue::BundleIds(vec![ + "com.apple.Notes".to_string(), + ])), + accessibility: Some(true), + calendar: Some(true), }), - ) + ..Default::default() + })) .expect("profile"); assert_eq!( @@ -473,8 +470,8 @@ mod tests { let skill_dir = tempdir.path().join("skill"); fs::create_dir_all(&skill_dir).expect("skill dir"); - let profile = compile_permission_profile(&skill_dir, Some(PermissionProfile::default())) - .expect("profile"); + let profile = + compile_permission_profile(Some(&SkillPermissionProfile::default())).expect("profile"); assert_eq!( profile.macos_seatbelt_profile_extensions, 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 084b245072..c12b4c00cb 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -241,7 +241,8 @@ impl CoreShellActionProvider { .or_else(|| { skill .permission_profile - .clone() + .as_ref() + .map(PermissionProfile::from) .map(EscalationPermissions::PermissionProfile) .map(EscalationExecution::Permissions) }) @@ -486,7 +487,10 @@ impl EscalationPolicy for CoreShellActionProvider { program, argv, workdir, - skill.permission_profile.clone(), + skill + .permission_profile + .as_ref() + .map(PermissionProfile::from), Self::skill_escalation_execution(&skill), decision_source, ) diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index c2c729be33..4fe7b3504b 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -291,6 +291,12 @@ permissions: .await?; let (script_path_str, command) = skill_script_command(&test, "hello-mbolin.sh")?; + let skill_dir = PathBuf::from(&script_path_str) + .parent() + .expect("skill script parent") + .parent() + .expect("skill root") + .to_path_buf(); let arguments = shell_command_arguments(&command)?; let mocks = mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command") @@ -331,8 +337,8 @@ permissions: approval.additional_permissions, Some(PermissionProfile { file_system: Some(FileSystemPermissions { - read: Some(vec![PathBuf::from("./data")]), - write: Some(vec![PathBuf::from("./output")]), + read: Some(vec![skill_dir.join("data")]), + write: Some(vec![skill_dir.join("output")]), }), ..Default::default() })