clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-06-01 19:02:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

core: remove special execve handling for skill scripts (#15812)

Browse Source
This commit is contained in:
Michael Bolin
2026-03-26 07:46:04 -07:00
committed by GitHub
parent 6dcac41d53
commit 01fa4f0212
7 changed files with 35 additions and 1073 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
codex-rs/core/src/tools/runtimes/mod.rs
View File

@@ -4,7 +4,6 @@ Module: runtimes
Concrete ToolRuntime implementations for specific tools. Each runtime stays
small and focused and reuses the orchestrator for approvals + sandbox + retry.
*/
use crate::SkillMetadata;
use crate::path_utils;
use crate::shell::Shell;
use crate::tools::sandboxing::ToolError;
@@ -17,14 +16,6 @@ pub mod apply_patch;
pub mod shell;
pub mod unified_exec;
#[derive(Debug, Clone)]
pub(crate) struct ExecveSessionApproval {
/// If this execve session approval is associated with a skill script, this
/// field contains metadata about the skill.
#[cfg_attr(not(unix), allow(dead_code))]
pub skill: Option<SkillMetadata>,
}
/// Shared helper to construct sandbox transform inputs from a tokenized command line.
/// Validates that at least a program is present.
pub(crate) fn build_sandbox_command(

188
codex-rs/core/src/tools/runtimes/shell/unix_escalation.rs
View File

@@ -1,5 +1,4 @@
use super::ShellRequest;
use crate::SkillMetadata;
use crate::error::CodexErr;
use crate::error::SandboxErr;
use crate::exec::ExecCapturePolicy;
@@ -13,8 +12,6 @@ use crate::sandboxing::ExecOptions;
use crate::sandboxing::ExecRequest;
use crate::sandboxing::SandboxPermissions;
use crate::shell::ShellType;
use crate::skills_load_input_from_config;
use crate::tools::runtimes::ExecveSessionApproval;
use crate::tools::runtimes::build_sandbox_command;
use crate::tools::sandboxing::SandboxAttempt;
use crate::tools::sandboxing::ToolCtx;
@@ -31,7 +28,6 @@ use codex_protocol::models::PermissionProfile;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::ExecApprovalRequestSkillMetadata;
use codex_protocol::protocol::NetworkPolicyRuleAction;
use codex_protocol::protocol::ReviewDecision;
use codex_protocol::protocol::SandboxPolicy;
@@ -74,9 +70,6 @@ const REJECT_SANDBOX_APPROVAL_REASON: &str =
"approval required by policy, but AskForApproval::Granular.sandbox_approval is false";
const REJECT_RULES_APPROVAL_REASON: &str =
"approval required by policy rule, but AskForApproval::Granular.rules is false";
const REJECT_SKILL_APPROVAL_REASON: &str =
"approval required by skill, but AskForApproval::Granular.skill_approval is false";
fn approval_sandbox_permissions(
sandbox_permissions: SandboxPermissions,
additional_permissions_preapproved: bool,
@@ -327,9 +320,6 @@ struct CoreShellActionProvider {
#[allow(clippy::large_enum_variant)]
enum DecisionSource {
SkillScript {
skill: SkillMetadata,
},
PrefixRule,
/// Often, this is `is_safe_command()`.
UnmatchedCommandFallback,
@@ -341,11 +331,6 @@ fn execve_prompt_is_rejected_by_policy(
) -> Option<&'static str> {
match (approval_policy, decision_source) {
(AskForApproval::Never, _) => Some(PROMPT_CONFLICT_REASON),
(AskForApproval::Granular(granular_config), DecisionSource::SkillScript { .. })
if !granular_config.allows_skill_approval() =>
{
Some(REJECT_SKILL_APPROVAL_REASON)
}
(AskForApproval::Granular(granular_config), DecisionSource::PrefixRule)
if !granular_config.allows_rules_approval() =>
{
@@ -397,17 +382,6 @@ impl CoreShellActionProvider {
}
}
fn skill_escalation_execution(skill: &SkillMetadata) -> EscalationExecution {
let permission_profile = skill.permission_profile.clone().unwrap_or_default();
if permission_profile.is_empty() {
EscalationExecution::TurnDefault
} else {
EscalationExecution::Permissions(EscalationPermissions::PermissionProfile(
permission_profile,
))
}
}
async fn prompt(
&self,
program: &AbsolutePathBuf,
@@ -415,7 +389,6 @@ impl CoreShellActionProvider {
workdir: &AbsolutePathBuf,
stopwatch: &Stopwatch,
additional_permissions: Option<PermissionProfile>,
decision_source: &DecisionSource,
) -> anyhow::Result<ReviewDecision> {
let command = join_program_and_argv(program, argv);
let workdir = workdir.to_path_buf();
@@ -442,28 +415,6 @@ impl CoreShellActionProvider {
)
.await;
}
let available_decisions = vec![
Some(ReviewDecision::Approved),
// Currently, ApprovedForSession is only honored for skills,
// so only offer it for skill script approvals.
if matches!(decision_source, DecisionSource::SkillScript { .. }) {
Some(ReviewDecision::ApprovedForSession)
} else {
None
},
Some(ReviewDecision::Abort),
]
.into_iter()
.flatten()
.collect();
let skill_metadata = match decision_source {
DecisionSource::SkillScript { skill } => {
Some(ExecApprovalRequestSkillMetadata {
path_to_skills_md: skill.path_to_skills_md.clone(),
})
}
DecisionSource::PrefixRule | DecisionSource::UnmatchedCommandFallback => None,
};
session
.request_command_approval(
&turn,
@@ -475,48 +426,14 @@ impl CoreShellActionProvider {
/*network_approval_context*/ None,
/*proposed_execpolicy_amendment*/ None,
additional_permissions,
skill_metadata,
Some(available_decisions),
/*skill_metadata*/ None,
Some(vec![ReviewDecision::Approved, ReviewDecision::Abort]),
)
.await
})
.await)
}
/// Because we should be intercepting execve(2) calls, `program` should be
/// an absolute path. The idea is that we check to see whether it matches
/// any skills.
async fn find_skill(&self, program: &AbsolutePathBuf) -> Option<SkillMetadata> {
let force_reload = false;
let turn_config = self.turn.config.as_ref();
let plugin_outcome = self
.session
.services
.plugins_manager
.plugins_for_config(turn_config);
let effective_skill_roots = plugin_outcome.effective_skill_roots();
let skills_input = skills_load_input_from_config(turn_config, effective_skill_roots);
let skills_outcome = self
.session
.services
.skills_manager
.skills_for_cwd(&skills_input, force_reload)
.await;
let program_path = program.as_path();
for skill in skills_outcome.skills {
// We intentionally ignore "enabled" status here for now.
let Some(skill_root) = skill.path_to_skills_md.parent() else {
continue;
};
if program_path.starts_with(skill_root.join("scripts")) {
return Some(skill);
}
}
None
}
#[allow(clippy::too_many_arguments)]
async fn process_decision(
&self,
@@ -540,17 +457,11 @@ impl CoreShellActionProvider {
EscalationDecision::deny(Some("Execution forbidden by policy".to_string()))
} else {
match self
.prompt(
program,
argv,
workdir,
&self.stopwatch,
prompt_permissions,
&decision_source,
)
.prompt(program, argv, workdir, &self.stopwatch, prompt_permissions)
.await?
{
ReviewDecision::Approved
| ReviewDecision::ApprovedForSession
| ReviewDecision::ApprovedExecpolicyAmendment { .. } => {
if needs_escalation {
EscalationDecision::escalate(escalation_execution.clone())
@@ -558,33 +469,6 @@ impl CoreShellActionProvider {
EscalationDecision::run()
}
}
ReviewDecision::ApprovedForSession => {
// Currently, we only add session approvals for
// skill scripts because we are storing only the
// `program` whereas prefix rules may be restricted by a longer prefix.
if let DecisionSource::SkillScript { skill } = decision_source {
tracing::debug!(
"Adding session approval for {program:?} due to user approval of skill script {skill:?}"
);
self.session
.services
.execve_session_approvals
.write()
.await
.insert(
program.clone(),
ExecveSessionApproval {
skill: Some(skill.clone()),
},
);
}
if needs_escalation {
EscalationDecision::escalate(escalation_execution.clone())
} else {
EscalationDecision::run()
}
}
ReviewDecision::NetworkPolicyAmendment {
network_policy_amendment,
} => match network_policy_amendment.action {
@@ -641,69 +525,6 @@ impl EscalationPolicy for CoreShellActionProvider {
"Determining escalation action for command {program:?} with args {argv:?} in {workdir:?}"
);
// Check to see whether `program` has an existing entry in
// `execve_session_approvals`. If so, we can skip policy checks and user
// prompts and go straight to allowing execution.
let approval = {
self.session
.services
.execve_session_approvals
.read()
.await
.get(program)
.cloned()
};
if let Some(approval) = approval {
tracing::debug!(
"Found session approval for {program:?}, allowing execution without further checks"
);
let execution = approval
.skill
.as_ref()
.map(Self::skill_escalation_execution)
.unwrap_or(EscalationExecution::TurnDefault);
return Ok(EscalationDecision::escalate(execution));
}
// In the usual case, the execve wrapper reports the command being
// executed in `program`, so a direct skill lookup is sufficient.
if let Some(skill) = self.find_skill(program).await {
// For now, scripts that look like they belong to skills bypass
// general exec policy evaluation. Permissionless skills inherit the
// turn sandbox directly; skills with declared permissions still
// prompt here before applying their permission profile.
let prompt_permissions = skill.permission_profile.clone();
if prompt_permissions
.as_ref()
.is_none_or(PermissionProfile::is_empty)
{
tracing::debug!(
"Matched {program:?} to permissionless skill {skill:?}, inheriting turn sandbox"
);
return Ok(EscalationDecision::escalate(
EscalationExecution::TurnDefault,
));
}
tracing::debug!("Matched {program:?} to skill {skill:?}, prompting for approval");
let needs_escalation = true;
let decision_source = DecisionSource::SkillScript {
skill: skill.clone(),
};
return self
.process_decision(
Decision::Prompt,
needs_escalation,
program,
argv,
workdir,
prompt_permissions,
Self::skill_escalation_execution(&skill),
decision_source,
)
.await;
}
let evaluation = {
let policy = self.policy.read().await;
evaluate_intercepted_exec_policy(
@@ -746,7 +567,6 @@ impl EscalationPolicy for CoreShellActionProvider {
.macos_seatbelt_profile_extensions
.as_ref(),
),
DecisionSource::SkillScript { .. } => unreachable!("handled above"),
};
self.process_decision(
evaluation.decision,

87
codex-rs/core/src/tools/runtimes/shell/unix_escalation_tests.rs
View File

@@ -8,7 +8,6 @@ use super::evaluate_intercepted_exec_policy;
use super::extract_shell_script;
use super::join_program_and_argv;
use super::map_exec_result;
use crate::SkillMetadata;
#[cfg(target_os = "macos")]
use crate::config::Constrained;
#[cfg(target_os = "macos")]
@@ -36,7 +35,6 @@ use codex_protocol::permissions::FileSystemSandboxEntry;
use codex_protocol::permissions::FileSystemSandboxPolicy;
use codex_protocol::permissions::FileSystemSpecialPath;
use codex_protocol::permissions::NetworkSandboxPolicy;
use codex_protocol::protocol::SkillScope;
use codex_sandboxing::SandboxType;
#[cfg(target_os = "macos")]
use codex_sandboxing::seatbelt::MACOS_PATH_TO_SEATBELT_EXECUTABLE;
@@ -83,55 +81,6 @@ fn unrestricted_file_system_sandbox_policy() -> FileSystemSandboxPolicy {
FileSystemSandboxPolicy::unrestricted()
}
fn test_skill_metadata(permission_profile: Option<PermissionProfile>) -> SkillMetadata {
SkillMetadata {
name: "skill".to_string(),
description: "description".to_string(),
short_description: None,
interface: None,
dependencies: None,
policy: None,
permission_profile,
managed_network_override: None,
path_to_skills_md: PathBuf::from("/tmp/skill/SKILL.md"),
scope: SkillScope::User,
}
}
#[test]
fn execve_prompt_rejection_uses_skill_approval_for_skill_scripts() {
let decision_source = super::DecisionSource::SkillScript {
skill: test_skill_metadata(None),
};
assert_eq!(
super::execve_prompt_is_rejected_by_policy(
AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: true,
request_permissions: true,
mcp_elicitations: true,
}),
&decision_source,
),
None,
);
assert_eq!(
super::execve_prompt_is_rejected_by_policy(
AskForApproval::Granular(GranularApprovalConfig {
sandbox_approval: true,
rules: true,
skill_approval: false,
request_permissions: true,
mcp_elicitations: true,
}),
&decision_source,
),
Some("approval required by skill, but AskForApproval::Granular.skill_approval is false"),
);
}
#[test]
fn execve_prompt_rejection_keeps_prefix_rules_on_rules_flag() {
assert_eq!(
@@ -392,42 +341,6 @@ fn shell_request_escalation_execution_is_explicit() {
);
}
#[test]
fn skill_escalation_execution_uses_additional_permissions() {
let requested_permissions = PermissionProfile {
file_system: Some(FileSystemPermissions {
read: None,
write: Some(vec![
AbsolutePathBuf::from_absolute_path("/tmp/output").unwrap(),
]),
}),
..Default::default()
};
assert_eq!(
CoreShellActionProvider::skill_escalation_execution(&test_skill_metadata(Some(
requested_permissions.clone(),
))),
EscalationExecution::Permissions(EscalationPermissions::PermissionProfile(
requested_permissions,
)),
);
}
#[test]
fn skill_escalation_execution_ignores_empty_permissions() {
assert_eq!(
CoreShellActionProvider::skill_escalation_execution(&test_skill_metadata(Some(
PermissionProfile::default(),
))),
EscalationExecution::TurnDefault,
);
assert_eq!(
CoreShellActionProvider::skill_escalation_execution(&test_skill_metadata(None)),
EscalationExecution::TurnDefault,
);
}
#[test]
fn evaluate_intercepted_exec_policy_uses_wrapper_command_when_shell_wrapper_parsing_disabled() {
let policy_src = r#"prefix_rule(pattern = ["npm", "publish"], decision = "prompt")"#;