diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 72ac2bfbd..a63aa2414 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1380,7 +1380,7 @@ impl Session { otel_manager, models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - execve_session_approvals: RwLock::new(HashSet::new()), + execve_session_approvals: RwLock::new(HashMap::new()), skills_manager, file_watcher, agent_control, @@ -8458,7 +8458,7 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - execve_session_approvals: RwLock::new(HashSet::new()), + execve_session_approvals: RwLock::new(HashMap::new()), skills_manager, file_watcher, agent_control, @@ -8617,7 +8617,7 @@ mod tests { otel_manager: otel_manager.clone(), models_manager: Arc::clone(&models_manager), tool_approvals: Mutex::new(ApprovalStore::default()), - execve_session_approvals: RwLock::new(HashSet::new()), + execve_session_approvals: RwLock::new(HashMap::new()), skills_manager, file_watcher, agent_control, diff --git a/codex-rs/core/src/state/service.rs b/codex-rs/core/src/state/service.rs index 0ec03b139..f501551f9 100644 --- a/codex-rs/core/src/state/service.rs +++ b/codex-rs/core/src/state/service.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::HashMap; use std::sync::Arc; use crate::AuthManager; @@ -14,6 +14,7 @@ use crate::models_manager::manager::ModelsManager; use crate::skills::SkillsManager; use crate::state_db::StateDbHandle; use crate::tools::network_approval::NetworkApprovalService; +use crate::tools::runtimes::ExecveSessionApproval; use crate::tools::sandboxing::ApprovalStore; use crate::unified_exec::UnifiedExecProcessManager; use codex_hooks::Hooks; @@ -45,7 +46,7 @@ pub(crate) struct SessionServices { pub(crate) otel_manager: OtelManager, pub(crate) tool_approvals: Mutex, #[cfg_attr(not(unix), allow(dead_code))] - pub(crate) execve_session_approvals: RwLock>, + pub(crate) execve_session_approvals: RwLock>, pub(crate) skills_manager: Arc, pub(crate) file_watcher: Arc, pub(crate) agent_control: AgentControl, diff --git a/codex-rs/core/src/tools/runtimes/mod.rs b/codex-rs/core/src/tools/runtimes/mod.rs index 937ab341d..51002f3ce 100644 --- a/codex-rs/core/src/tools/runtimes/mod.rs +++ b/codex-rs/core/src/tools/runtimes/mod.rs @@ -9,6 +9,7 @@ use crate::path_utils; use crate::sandboxing::CommandSpec; use crate::sandboxing::SandboxPermissions; use crate::shell::Shell; +use crate::skills::SkillMetadata; use crate::tools::sandboxing::ToolError; use codex_protocol::models::PermissionProfile; use std::collections::HashMap; @@ -18,6 +19,14 @@ 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, +} + /// Shared helper to construct a CommandSpec from a tokenized command line. /// Validates that at least a program is present. pub(crate) fn build_command_spec( 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 83f4fa32a..3b1a0c906 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::tools::runtimes::ExecveSessionApproval; use crate::tools::runtimes::build_command_spec; use crate::tools::sandboxing::SandboxAttempt; use crate::tools::sandboxing::ToolCtx; @@ -163,8 +164,11 @@ struct CoreShellActionProvider { stopwatch: Stopwatch, } +#[allow(clippy::large_enum_variant)] enum DecisionSource { - SkillScript, + SkillScript { + skill: SkillMetadata, + }, PrefixRule, /// Often, this is `is_safe_command()`. UnmatchedCommandFallback, @@ -284,13 +288,21 @@ impl CoreShellActionProvider { // 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 matches!(decision_source, DecisionSource::SkillScript) { + 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()); + .insert( + program.clone(), + ExecveSessionApproval { + skill: Some(skill.clone()), + }, + ); } if needs_escalation { @@ -349,6 +361,33 @@ 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" + ); + // TODO(mbolin): We need to include the permissions with the + // escalation decision so it can be run with the appropriate + // permissions. + let _permissions = approval + .skill + .as_ref() + .and_then(|s| s.permission_profile.clone()); + + return Ok(EscalateAction::Escalate); + } + // 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 { @@ -356,32 +395,19 @@ impl EscalationPolicy for CoreShellActionProvider { // to skills, which means we ignore exec policy rules for those // scripts. tracing::debug!("Matched {program:?} to skill {skill:?}, prompting for approval"); - // TODO(mbolin): We should read the permissions associated with the - // skill and use those specific permissions in the - // EscalateAction::Run case, rather than always escalating when a - // skill matches. let needs_escalation = true; - let is_approved_for_session = self - .session - .services - .execve_session_approvals - .read() - .await - .contains(program); - let decision = if is_approved_for_session { - Decision::Allow - } else { - Decision::Prompt + let decision_source = DecisionSource::SkillScript { + skill: skill.clone(), }; return self .process_decision( - decision, + Decision::Prompt, needs_escalation, program, argv, workdir, skill.permission_profile.clone(), - DecisionSource::SkillScript, + decision_source, ) .await; }