diff --git a/codex-rs/core/src/mcp/skill_dependencies.rs b/codex-rs/core/src/mcp/skill_dependencies.rs index 4a1af423f..f15bb6ec5 100644 --- a/codex-rs/core/src/mcp/skill_dependencies.rs +++ b/codex-rs/core/src/mcp/skill_dependencies.rs @@ -442,7 +442,6 @@ mod tests { dependencies: Some(SkillDependencies { tools }), policy: None, permission_profile: None, - permissions: None, path_to_skills_md: PathBuf::from("skill"), scope: SkillScope::User, } diff --git a/codex-rs/core/src/skills/injection.rs b/codex-rs/core/src/skills/injection.rs index 9bb3a7478..e293aac59 100644 --- a/codex-rs/core/src/skills/injection.rs +++ b/codex-rs/core/src/skills/injection.rs @@ -483,7 +483,6 @@ mod tests { dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: PathBuf::from(path), scope: codex_protocol::protocol::SkillScope::User, } diff --git a/codex-rs/core/src/skills/invocation_utils.rs b/codex-rs/core/src/skills/invocation_utils.rs index d2831c4df..36bb0c3ca 100644 --- a/codex-rs/core/src/skills/invocation_utils.rs +++ b/codex-rs/core/src/skills/invocation_utils.rs @@ -253,7 +253,6 @@ mod tests { dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: skill_doc_path, scope: codex_protocol::protocol::SkillScope::User, } diff --git a/codex-rs/core/src/skills/loader.rs b/codex-rs/core/src/skills/loader.rs index 110e7fa95..bfa22cf01 100644 --- a/codex-rs/core/src/skills/loader.rs +++ b/codex-rs/core/src/skills/loader.rs @@ -1,4 +1,3 @@ -use crate::config::Permissions; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigLayerStackOrdering; use crate::config_loader::default_project_root_markers; @@ -12,7 +11,6 @@ use crate::skills::model::SkillLoadOutcome; use crate::skills::model::SkillMetadata; use crate::skills::model::SkillPolicy; use crate::skills::model::SkillToolDependency; -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; @@ -69,7 +67,6 @@ struct LoadedSkillMetadata { dependencies: Option, policy: Option, permission_profile: Option, - permissions: Option, } #[derive(Debug, Default, Deserialize)] @@ -528,7 +525,6 @@ fn parse_skill_file(path: &Path, scope: SkillScope) -> Result Result LoadedSkillMetadata { policy, permissions, } = parsed; - let permission_profile = permissions.clone().filter(|profile| !profile.is_empty()); - LoadedSkillMetadata { interface: resolve_interface(interface, skill_dir), dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), - permission_profile, - permissions: compile_permission_profile(skill_dir, permissions), + permission_profile: permissions.filter(|profile| !profile.is_empty()), } } @@ -864,9 +856,7 @@ mod tests { use crate::config::ConfigBuilder; use crate::config::ConfigOverrides; use crate::config::ConfigToml; - use crate::config::Constrained; use crate::config::ProjectConfig; - use crate::config::types::ShellEnvironmentPolicy; use crate::config_loader::ConfigLayerEntry; use crate::config_loader::ConfigLayerStack; use crate::config_loader::ConfigRequirements; @@ -1100,7 +1090,6 @@ mod tests { dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1258,7 +1247,6 @@ mod tests { }), policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1315,7 +1303,6 @@ interface: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(skill_path.as_path()), scope: SkillScope::User, }] @@ -1435,44 +1422,6 @@ permissions: macos: None, }) ); - #[cfg(target_os = "macos")] - let macos_seatbelt_profile_extensions = - Some(crate::seatbelt_permissions::MacOsSeatbeltProfileExtensions::default()); - #[cfg(not(target_os = "macos"))] - let macos_seatbelt_profile_extensions = None; - assert_eq!( - outcome.skills[0].permissions, - Some(Permissions { - approval_policy: Constrained::allow_any(crate::protocol::AskForApproval::Never), - sandbox_policy: Constrained::allow_any( - crate::protocol::SandboxPolicy::WorkspaceWrite { - writable_roots: vec![ - AbsolutePathBuf::try_from(normalized( - skill_dir.join("output").as_path(), - )) - .expect("absolute output path") - ], - read_only_access: crate::protocol::ReadOnlyAccess::Restricted { - include_platform_defaults: true, - readable_roots: vec![ - AbsolutePathBuf::try_from(normalized( - skill_dir.join("data").as_path(), - )) - .expect("absolute data path") - ], - }, - network_access: true, - exclude_tmpdir_env_var: false, - exclude_slash_tmp: false, - } - ), - network: None, - allow_login_shell: true, - shell_environment_policy: ShellEnvironmentPolicy::default(), - windows_sandbox_mode: None, - macos_seatbelt_profile_extensions, - }) - ); } #[tokio::test] @@ -1497,34 +1446,7 @@ permissions: {} outcome.errors ); assert_eq!(outcome.skills.len(), 1); - #[cfg(target_os = "macos")] - let expected = Some(Permissions { - approval_policy: Constrained::allow_any(crate::protocol::AskForApproval::Never), - sandbox_policy: Constrained::allow_any( - crate::protocol::SandboxPolicy::new_read_only_policy(), - ), - network: None, - allow_login_shell: true, - shell_environment_policy: ShellEnvironmentPolicy::default(), - windows_sandbox_mode: None, - macos_seatbelt_profile_extensions: Some( - crate::seatbelt_permissions::MacOsSeatbeltProfileExtensions::default(), - ), - }); - #[cfg(not(target_os = "macos"))] - let expected = Some(Permissions { - approval_policy: Constrained::allow_any(crate::protocol::AskForApproval::Never), - sandbox_policy: Constrained::allow_any( - crate::protocol::SandboxPolicy::new_read_only_policy(), - ), - network: None, - allow_login_shell: true, - shell_environment_policy: ShellEnvironmentPolicy::default(), - windows_sandbox_mode: None, - macos_seatbelt_profile_extensions: None, - }); assert_eq!(outcome.skills[0].permission_profile, None); - assert_eq!(outcome.skills[0].permissions, expected); } #[cfg(target_os = "macos")] @@ -1556,24 +1478,21 @@ permissions: outcome.errors ); assert_eq!(outcome.skills.len(), 1); - let profile = outcome.skills[0] - .permissions - .as_ref() - .expect("permission profile"); assert_eq!( - profile.macos_seatbelt_profile_extensions, - Some( - crate::seatbelt_permissions::MacOsSeatbeltProfileExtensions { - macos_preferences: - crate::seatbelt_permissions::MacOsPreferencesPermission::ReadWrite, - macos_automation: - crate::seatbelt_permissions::MacOsAutomationPermission::BundleIds(vec![ - "com.apple.Notes".to_string() - ],), - macos_accessibility: true, - macos_calendar: true, - } - ) + outcome.skills[0].permission_profile, + Some(PermissionProfile { + macos: Some(codex_protocol::models::MacOsPermissions { + preferences: Some(codex_protocol::models::MacOsPreferencesValue::Mode( + "readwrite".to_string(), + ),), + automations: Some(codex_protocol::models::MacOsAutomationValue::BundleIds( + vec!["com.apple.Notes".to_string()], + )), + accessibility: Some(true), + calendar: Some(true), + }), + ..Default::default() + }) ); } @@ -1607,17 +1526,19 @@ permissions: ); assert_eq!(outcome.skills.len(), 1); assert_eq!( - outcome.skills[0].permissions, - Some(Permissions { - approval_policy: Constrained::allow_any(crate::protocol::AskForApproval::Never), - sandbox_policy: Constrained::allow_any( - crate::protocol::SandboxPolicy::new_read_only_policy(), - ), - network: None, - allow_login_shell: true, - shell_environment_policy: ShellEnvironmentPolicy::default(), - windows_sandbox_mode: None, - macos_seatbelt_profile_extensions: None, + outcome.skills[0].permission_profile, + Some(PermissionProfile { + macos: Some(codex_protocol::models::MacOsPermissions { + preferences: Some(codex_protocol::models::MacOsPreferencesValue::Mode( + "readwrite".to_string(), + )), + automations: Some(codex_protocol::models::MacOsAutomationValue::BundleIds( + vec!["com.apple.Notes".to_string()], + )), + accessibility: Some(true), + calendar: Some(true), + }), + ..Default::default() }) ); } @@ -1667,7 +1588,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1709,7 +1629,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1764,7 +1683,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1807,7 +1725,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1853,7 +1770,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::User, }] @@ -1915,7 +1831,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -1953,7 +1868,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&shared_skill_path), scope: SkillScope::Admin, }] @@ -1995,7 +1909,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&linked_skill_path), scope: SkillScope::Repo, }] @@ -2064,7 +1977,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&within_depth_path), scope: SkillScope::User, }] @@ -2093,7 +2005,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -2127,7 +2038,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -2170,7 +2080,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -2203,7 +2112,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::User, }] @@ -2317,7 +2225,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -2354,7 +2261,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -2409,7 +2315,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&nested_skill_path), scope: SkillScope::Repo, }, @@ -2421,7 +2326,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&root_skill_path), scope: SkillScope::Repo, }, @@ -2462,7 +2366,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -2501,7 +2404,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -2544,7 +2446,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&repo_skill_path), scope: SkillScope::Repo, }, @@ -2556,7 +2457,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&user_skill_path), scope: SkillScope::User, }, @@ -2622,7 +2522,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: first_path, scope: SkillScope::Repo, }, @@ -2634,7 +2533,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: second_path, scope: SkillScope::Repo, }, @@ -2707,7 +2605,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::Repo, }] @@ -2767,7 +2664,6 @@ permissions: dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: normalized(&skill_path), scope: SkillScope::System, }] diff --git a/codex-rs/core/src/skills/model.rs b/codex-rs/core/src/skills/model.rs index ddb1dd263..525ea28da 100644 --- a/codex-rs/core/src/skills/model.rs +++ b/codex-rs/core/src/skills/model.rs @@ -3,7 +3,6 @@ use std::collections::HashSet; use std::path::PathBuf; use std::sync::Arc; -use crate::config::Permissions; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SkillScope; @@ -16,8 +15,6 @@ pub struct SkillMetadata { pub dependencies: Option, pub policy: Option, pub permission_profile: Option, - // This is an experimental field. - pub permissions: Option, /// Path to the SKILLS.md file that declares this skill. pub path_to_skills_md: PathBuf, pub scope: SkillScope, diff --git a/codex-rs/core/src/skills/permissions.rs b/codex-rs/core/src/skills/permissions.rs index 38d02ee0b..1d0e6c82a 100644 --- a/codex-rs/core/src/skills/permissions.rs +++ b/codex-rs/core/src/skills/permissions.rs @@ -1,26 +1,42 @@ +#[cfg(any(unix, test))] use std::collections::HashSet; -use std::path::Path; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsAutomationValue; +#[cfg(any(unix, test))] use codex_protocol::models::MacOsPermissions; #[cfg(target_os = "macos")] use codex_protocol::models::MacOsPreferencesValue; +#[cfg(any(unix, test))] use codex_protocol::models::MacOsSeatbeltProfileExtensions; +#[cfg(any(unix, test))] use codex_protocol::models::PermissionProfile; +#[cfg(any(unix, test))] use codex_utils_absolute_path::AbsolutePathBuf; +#[cfg(any(unix, test))] use dunce::canonicalize as canonicalize_path; +#[cfg(any(unix, test))] use tracing::warn; +#[cfg(any(unix, test))] use crate::config::Constrained; +#[cfg(any(unix, test))] use crate::config::Permissions; +#[cfg(any(unix, test))] use crate::config::types::ShellEnvironmentPolicy; +#[cfg(any(unix, test))] use crate::protocol::AskForApproval; +#[cfg(any(unix, test))] use crate::protocol::ReadOnlyAccess; +#[cfg(any(unix, test))] use crate::protocol::SandboxPolicy; +/// Compiles a skill `PermissionProfile` for the Unix shell escalation path. +/// +/// Normal Windows builds do not currently call this helper, so it is only +/// compiled on Unix and in tests. +#[cfg(any(unix, test))] pub(crate) fn compile_permission_profile( - _skill_dir: &Path, permissions: Option, ) -> Option { let PermissionProfile { @@ -78,6 +94,7 @@ pub(crate) fn compile_permission_profile( }) } +#[cfg(any(unix, test))] fn normalize_permission_paths(values: &[AbsolutePathBuf], field: &str) -> Vec { let mut paths = Vec::new(); let mut seen = HashSet::new(); @@ -94,6 +111,7 @@ fn normalize_permission_paths(values: &[AbsolutePathBuf], field: &str) -> Vec Option { let canonicalized = canonicalize_path(value.as_path()).unwrap_or_else(|_| value.to_path_buf()); match AbsolutePathBuf::from_absolute_path(&canonicalized) { @@ -184,7 +202,7 @@ fn resolve_macos_automation_permission( } } -#[cfg(not(target_os = "macos"))] +#[cfg(all(not(target_os = "macos"), any(unix, test)))] fn build_macos_seatbelt_profile_extensions( _: &MacOsPermissions, ) -> Option { @@ -225,21 +243,18 @@ 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![ - absolute_path(&skill_dir.join("data")), - absolute_path(&skill_dir.join("data")), - absolute_path(&skill_dir.join("scripts/../data")), - ]), - write: Some(vec![absolute_path(&skill_dir.join("output"))]), - }), - ..Default::default() + let profile = compile_permission_profile(Some(PermissionProfile { + network: Some(true), + file_system: Some(FileSystemPermissions { + read: Some(vec![ + absolute_path(&skill_dir.join("data")), + absolute_path(&skill_dir.join("data")), + absolute_path(&skill_dir.join("scripts/../data")), + ]), + write: Some(vec![absolute_path(&skill_dir.join("output"))]), }), - ) + ..Default::default() + })) .expect("profile"); assert_eq!( @@ -284,7 +299,7 @@ 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, None); + let profile = compile_permission_profile(None); assert_eq!(profile, None); } @@ -295,13 +310,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(PermissionProfile { + network: Some(true), + ..Default::default() + })) .expect("profile"); assert_eq!( @@ -330,17 +342,14 @@ 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![absolute_path(&skill_dir.join("data"))]), - write: Some(Vec::new()), - }), - ..Default::default() + let profile = compile_permission_profile(Some(PermissionProfile { + network: Some(true), + file_system: Some(FileSystemPermissions { + read: Some(vec![absolute_path(&skill_dir.join("data"))]), + write: Some(Vec::new()), }), - ) + ..Default::default() + })) .expect("profile"); assert_eq!( @@ -379,20 +388,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(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() + })) .expect("profile"); assert_eq!( @@ -419,8 +425,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(PermissionProfile::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 5288fd8e8..36f63be4f 100644 --- a/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs +++ b/codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs @@ -9,6 +9,7 @@ use crate::features::Feature; use crate::sandboxing::SandboxPermissions; use crate::shell::ShellType; use crate::skills::SkillMetadata; +use crate::skills::permissions::compile_permission_profile; use crate::tools::runtimes::ExecveSessionApproval; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::SandboxAttempt; @@ -227,9 +228,7 @@ impl CoreShellActionProvider { } fn skill_escalation_execution(skill: &SkillMetadata) -> EscalationExecution { - skill - .permissions - .as_ref() + compile_permission_profile(skill.permission_profile.clone()) .map(|permissions| { EscalationExecution::Permissions(EscalationPermissions::Permissions( EscalatedPermissions { @@ -240,13 +239,6 @@ impl CoreShellActionProvider { }, )) }) - .or_else(|| { - skill - .permission_profile - .clone() - .map(EscalationPermissions::PermissionProfile) - .map(EscalationExecution::Permissions) - }) .unwrap_or(EscalationExecution::TurnDefault) } diff --git a/codex-rs/core/tests/suite/skill_approval.rs b/codex-rs/core/tests/suite/skill_approval.rs index 84e1d2408..c645c06d1 100644 --- a/codex-rs/core/tests/suite/skill_approval.rs +++ b/codex-rs/core/tests/suite/skill_approval.rs @@ -404,6 +404,140 @@ async fn shell_zsh_fork_skill_without_permissions_inherits_turn_sandbox() -> Res Ok(()) } +/// Empty skill permissions should behave like no skill override and inherit the +/// turn sandbox instead of forcing an explicit read-only skill sandbox. +#[cfg(unix)] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn shell_zsh_fork_skill_with_empty_permissions_inherits_turn_sandbox() -> Result<()> { + skip_if_no_network!(Ok(())); + + let Some(runtime) = zsh_fork_runtime("zsh-fork empty skill permissions test")? else { + return Ok(()); + }; + + let outside_dir = tempfile::tempdir_in(std::env::current_dir()?)?; + let outside_path = outside_dir + .path() + .join("zsh-fork-skill-empty-permissions.txt"); + let outside_path_quoted = shlex::try_join([outside_path.to_string_lossy().as_ref()])?; + let script_contents = format!( + "#!/bin/sh\nprintf '%s' allowed > {outside_path_quoted}\ncat {outside_path_quoted}\n" + ); + let outside_path_for_hook = outside_path.clone(); + let script_contents_for_hook = script_contents.clone(); + + let server = start_mock_server().await; + let test = build_zsh_fork_test( + &server, + runtime, + AskForApproval::OnRequest, + SandboxPolicy::DangerFullAccess, + move |home| { + let _ = fs::remove_file(&outside_path_for_hook); + write_skill_with_shell_script_contents( + home, + "mbolin-test-skill", + "sandboxed.sh", + &script_contents_for_hook, + ) + .unwrap(); + write_skill_metadata(home, "mbolin-test-skill", "permissions: {}\n").unwrap(); + }, + ) + .await?; + + let (script_path_str, command) = skill_script_command(&test, "sandboxed.sh")?; + + let first_call_id = "zsh-fork-skill-empty-permissions-1"; + let first_arguments = shell_command_arguments(&command)?; + let first_mocks = mount_function_call_agent_response( + &server, + first_call_id, + &first_arguments, + "shell_command", + ) + .await; + + submit_turn_with_policies( + &test, + "use $mbolin-test-skill", + AskForApproval::OnRequest, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let approval = wait_for_exec_approval_request(&test) + .await + .expect("expected exec approval request before completion"); + assert_eq!(approval.call_id, first_call_id); + assert_eq!(approval.command, vec![script_path_str.clone()]); + assert_eq!(approval.additional_permissions, None); + + test.codex + .submit(Op::ExecApproval { + id: approval.effective_approval_id(), + turn_id: None, + decision: ReviewDecision::ApprovedForSession, + }) + .await?; + + wait_for_turn_complete(&test).await; + + let first_output = first_mocks + .completion + .single_request() + .function_call_output(first_call_id)["output"] + .as_str() + .unwrap_or_default() + .to_string(); + assert!( + first_output.contains("allowed"), + "expected empty skill permissions to inherit full-access turn sandbox, got output: {first_output:?}" + ); + assert_eq!(fs::read_to_string(&outside_path)?, "allowed"); + + let second_call_id = "zsh-fork-skill-empty-permissions-2"; + let second_arguments = shell_command_arguments(&command)?; + let second_mocks = mount_function_call_agent_response( + &server, + second_call_id, + &second_arguments, + "shell_command", + ) + .await; + + let _ = fs::remove_file(&outside_path); + + submit_turn_with_policies( + &test, + "use $mbolin-test-skill", + AskForApproval::OnRequest, + SandboxPolicy::DangerFullAccess, + ) + .await?; + + let cached_approval = wait_for_exec_approval_request(&test).await; + assert!( + cached_approval.is_none(), + "expected second run to reuse the cached session approval" + ); + + let second_output = second_mocks + .completion + .single_request() + .function_call_output(second_call_id)["output"] + .as_str() + .unwrap_or_default() + .to_string(); + assert!( + second_output.contains("allowed"), + "expected cached empty-permissions skill approval to inherit the turn sandbox, got output: {second_output:?}" + ); + assert_eq!(fs::read_to_string(&outside_path)?, "allowed"); + + Ok(()) +} + /// The validation to focus on is: writes to the skill-approved folder succeed, /// and writes to an unrelated folder fail, both before and after cached approval. #[cfg(unix)] diff --git a/codex-rs/tui/src/bottom_pane/mod.rs b/codex-rs/tui/src/bottom_pane/mod.rs index ac903d996..30f8f673a 100644 --- a/codex-rs/tui/src/bottom_pane/mod.rs +++ b/codex-rs/tui/src/bottom_pane/mod.rs @@ -1539,7 +1539,6 @@ mod tests { dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: PathBuf::from("test-skill"), scope: SkillScope::User, }]), diff --git a/codex-rs/tui/src/chatwidget/skills.rs b/codex-rs/tui/src/chatwidget/skills.rs index a30e7954f..efca42872 100644 --- a/codex-rs/tui/src/chatwidget/skills.rs +++ b/codex-rs/tui/src/chatwidget/skills.rs @@ -191,7 +191,6 @@ fn protocol_skill_to_core(skill: &ProtocolSkillMetadata) -> SkillMetadata { }), policy: None, permission_profile: None, - permissions: None, path_to_skills_md: skill.path.clone(), scope: skill.scope, } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index bfd7b4c44..e263e4511 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -936,7 +936,6 @@ async fn submission_prefers_selected_duplicate_skill_path() { dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: repo_skill_path, scope: SkillScope::Repo, }, @@ -948,7 +947,6 @@ async fn submission_prefers_selected_duplicate_skill_path() { dependencies: None, policy: None, permission_profile: None, - permissions: None, path_to_skills_md: user_skill_path.clone(), scope: SkillScope::User, },