From daf0f03ac80182e500f8731f42ed50daae9400ee Mon Sep 17 00:00:00 2001 From: pakrym-oai Date: Tue, 24 Feb 2026 12:13:20 -0800 Subject: [PATCH] Ensure shell command skills trigger approval (#12697) Summary - detect skill-invoking shell commands based on the original command string, request approvals when needed, and cache positive decisions per session - keep implicit skill invocation emitted after approval and keep skill approval decline messaging centralized to the shell handler - expand and adjust skill approval tests to cover shell-based skill scripts while matching the new detection expectations Testing - Not run (not requested) --- .../app-server/src/codex_message_processor.rs | 4 +- .../tests/suite/v2/skill_approval.rs | 99 ++++-- codex-rs/core/src/codex.rs | 39 +-- codex-rs/core/src/exec.rs | 8 + codex-rs/core/src/skills/invocation_utils.rs | 152 ++++++++- codex-rs/core/src/skills/mod.rs | 2 + codex-rs/core/src/tools/handlers/shell.rs | 34 +- codex-rs/core/tests/suite/exec.rs | 1 + codex-rs/core/tests/suite/skill_approval.rs | 319 +++++++++++++++--- .../linux-sandbox/tests/suite/landlock.rs | 2 + 10 files changed, 540 insertions(+), 120 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 9e53acd63..a868b2d41 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1758,8 +1758,10 @@ impl CodexMessageProcessor { None => None, }; let windows_sandbox_level = WindowsSandboxLevel::from_config(&self.config); + let command = params.command; let exec_params = ExecParams { - command: params.command, + original_command: command.join(" "), + command, cwd, expiration: timeout_ms.into(), env, diff --git a/codex-rs/app-server/tests/suite/v2/skill_approval.rs b/codex-rs/app-server/tests/suite/v2/skill_approval.rs index 945a545a8..a86e45976 100644 --- a/codex-rs/app-server/tests/suite/v2/skill_approval.rs +++ b/codex-rs/app-server/tests/suite/v2/skill_approval.rs @@ -3,6 +3,7 @@ use app_test_support::McpProcess; use app_test_support::create_final_assistant_message_sse_response; use app_test_support::create_mock_responses_server_sequence; use app_test_support::to_response; +use app_test_support::write_mock_responses_config_toml; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; use codex_app_server_protocol::ServerRequest; @@ -11,19 +12,72 @@ use codex_app_server_protocol::ThreadStartResponse; use codex_app_server_protocol::TurnStartParams; use codex_app_server_protocol::TurnStartResponse; use codex_app_server_protocol::UserInput as V2UserInput; +use codex_core::features::Feature; +use core_test_support::responses; +use pretty_assertions::assert_eq; +use serde_json::json; +use std::collections::BTreeMap; +use std::fs; +use std::path::Path; use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); +fn write_skill_with_script( + home: &Path, + name: &str, + script_body: &str, +) -> Result { + let skill_dir = home.join("skills").join(name); + let scripts_dir = skill_dir.join("scripts"); + fs::create_dir_all(&scripts_dir)?; + fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {name} skill\n---\n"), + )?; + let script_path = scripts_dir.join("run.py"); + fs::write(&script_path, script_body)?; + Ok(script_path) +} + +fn shell_command_response(tool_call_id: &str, command: &str) -> Result { + let arguments = serde_json::to_string(&json!({ + "command": command, + "timeout_ms": 500, + }))?; + Ok(responses::sse(vec![ + responses::ev_response_created("resp-1"), + responses::ev_function_call(tool_call_id, "shell_command", &arguments), + responses::ev_completed("resp-1"), + ])) +} + +fn command_for_script(script_path: &Path) -> Result { + let runner = if cfg!(windows) { "python" } else { "python3" }; + let script_path = script_path.to_string_lossy().into_owned(); + Ok(shlex::try_join([runner, script_path.as_str()])?) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn skill_request_approval_round_trip() -> Result<()> { +async fn skill_request_approval_round_trip_on_shell_command_skill_script_exec() -> Result<()> { let codex_home = tempfile::TempDir::new()?; - let server = - create_mock_responses_server_sequence(vec![create_final_assistant_message_sse_response( - "done", - )?]) - .await; - create_config_toml(codex_home.path(), &server.uri())?; + let script_path = write_skill_with_script(codex_home.path(), "demo", "print('hello')")?; + let tool_call_id = "skill-call"; + let command = command_for_script(&script_path)?; + let server = create_mock_responses_server_sequence(vec![ + shell_command_response(tool_call_id, &command)?, + create_final_assistant_message_sse_response("done")?, + ]) + .await; + write_mock_responses_config_toml( + codex_home.path(), + &server.uri(), + &BTreeMap::from([(Feature::SkillApproval, true)]), + 8192, + Some(false), + "mock_provider", + "compact", + )?; let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -57,7 +111,7 @@ async fn skill_request_approval_round_trip() -> Result<()> { mcp.read_stream_until_response_message(RequestId::Integer(turn_start_id)), ) .await??; - let TurnStartResponse { turn, .. } = to_response(turn_start_resp)?; + let TurnStartResponse { .. } = to_response::(turn_start_resp)?; let server_req = timeout( DEFAULT_READ_TIMEOUT, @@ -68,8 +122,8 @@ async fn skill_request_approval_round_trip() -> Result<()> { panic!("expected SkillRequestApproval request, got: {server_req:?}"); }; - assert_eq!(params.item_id, turn.id); - assert_eq!(params.skill_name, "test-skill"); + assert_eq!(params.item_id, tool_call_id); + assert_eq!(params.skill_name, "demo"); mcp.send_response(request_id, serde_json::json!({ "decision": "approve" })) .await?; @@ -82,28 +136,3 @@ async fn skill_request_approval_round_trip() -> Result<()> { Ok(()) } - -fn create_config_toml(codex_home: &std::path::Path, server_uri: &str) -> std::io::Result<()> { - let config_toml = codex_home.join("config.toml"); - std::fs::write( - config_toml, - format!( - r#" -model = "mock-model" -approval_policy = "never" -sandbox_mode = "read-only" - -model_provider = "mock_provider" - -[features] -skill_approval = true - -[model_providers.mock_provider] -name = "Mock provider for test" -base_url = "{server_uri}/v1" -request_max_retries = 0 -stream_max_retries = 0 -"# - ), - ) -} diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index dd4379459..012ae96b3 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4782,15 +4782,6 @@ pub(crate) async fn run_turn( collaboration_mode_kind: turn_context.collaboration_mode.mode, }); sess.send_event(&turn_context, event).await; - if turn_context.config.features.enabled(Feature::SkillApproval) { - let _ = sess - .request_skill_approval( - turn_context.as_ref(), - turn_context.sub_id.clone(), - "test-skill".to_string(), - ) - .await; - } // TODO(ccunningham): Pre-turn compaction runs before context updates and the // new user message are recorded. Estimate pending incoming items (context // diffs/full reinjection + user input) and trigger compaction preemptively @@ -9213,20 +9204,23 @@ mod tests { let timeout_ms = 1000; let sandbox_permissions = SandboxPermissions::RequireEscalated; + let command = if cfg!(windows) { + vec![ + "cmd.exe".to_string(), + "/C".to_string(), + "echo hi".to_string(), + ] + } else { + vec![ + "/bin/sh".to_string(), + "-c".to_string(), + "echo hi".to_string(), + ] + }; let params = ExecParams { - command: if cfg!(windows) { - vec![ - "cmd.exe".to_string(), - "/C".to_string(), - "echo hi".to_string(), - ] - } else { - vec![ - "/bin/sh".to_string(), - "-c".to_string(), - "echo hi".to_string(), - ] - }, + command: command.clone(), + original_command: shlex::try_join(command.iter().map(String::as_str)) + .unwrap_or_else(|_| command.join(" ")), cwd: turn_context.cwd.clone(), expiration: timeout_ms.into(), env: HashMap::new(), @@ -9240,6 +9234,7 @@ mod tests { let params2 = ExecParams { sandbox_permissions: SandboxPermissions::UseDefault, command: params.command.clone(), + original_command: params.original_command.clone(), cwd: params.cwd.clone(), expiration: timeout_ms.into(), env: HashMap::new(), diff --git a/codex-rs/core/src/exec.rs b/codex-rs/core/src/exec.rs index 288ee7f21..1d1169303 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -62,6 +62,7 @@ pub(crate) const MAX_EXEC_OUTPUT_DELTAS_PER_CALL: usize = 10_000; #[derive(Debug)] pub struct ExecParams { pub command: Vec, + pub original_command: String, pub cwd: PathBuf, pub expiration: ExecExpiration, pub env: HashMap, @@ -180,6 +181,7 @@ pub async fn process_exec_tool_call( let ExecParams { command, + original_command: _, cwd, mut env, expiration, @@ -249,6 +251,8 @@ pub(crate) async fn execute_exec_env( } = env; let params = ExecParams { + original_command: shlex::try_join(command.iter().map(String::as_str)) + .unwrap_or_else(|_| command.join(" ")), command, cwd, expiration, @@ -1121,6 +1125,8 @@ mod tests { ]; let env: HashMap = std::env::vars().collect(); let params = ExecParams { + original_command: shlex::try_join(command.iter().map(String::as_str)) + .unwrap_or_else(|_| command.join(" ")), command, cwd: std::env::current_dir()?, expiration: 500.into(), @@ -1174,6 +1180,8 @@ mod tests { let cancel_token = CancellationToken::new(); let cancel_tx = cancel_token.clone(); let params = ExecParams { + original_command: shlex::try_join(command.iter().map(String::as_str)) + .unwrap_or_else(|_| command.join(" ")), command, cwd: cwd.clone(), expiration: ExecExpiration::Cancellation(cancel_token), diff --git a/codex-rs/core/src/skills/invocation_utils.rs b/codex-rs/core/src/skills/invocation_utils.rs index 8c67dc633..fd847091d 100644 --- a/codex-rs/core/src/skills/invocation_utils.rs +++ b/codex-rs/core/src/skills/invocation_utils.rs @@ -7,8 +7,21 @@ use crate::analytics_client::SkillInvocation; use crate::analytics_client::build_track_events_context; use crate::codex::Session; use crate::codex::TurnContext; +use crate::features::Feature; use crate::skills::SkillLoadOutcome; use crate::skills::SkillMetadata; +use codex_protocol::protocol::ReviewDecision; +use serde::Serialize; + +pub(crate) const SKILL_APPROVAL_DECLINED_MESSAGE: &str = + "This script is part of the skill and the user declined the skill usage"; + +#[derive(Debug, Serialize)] +struct SkillApprovalCacheKey { + skill_name: String, + skill_path: PathBuf, + skill_scope: codex_protocol::protocol::SkillScope, +} pub(crate) fn build_implicit_skill_path_indexes( skills: Vec, @@ -41,8 +54,11 @@ fn detect_implicit_skill_invocation_for_command( let workdir = normalize_path(workdir.as_path()); let tokens = tokenize_command(command); - if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), workdir.as_path()) - { + if let Some(candidate) = detect_implicit_skill_script_invocation_for_tokens( + outcome, + tokens.as_slice(), + workdir.as_path(), + ) { return Some(candidate); } @@ -53,6 +69,82 @@ fn detect_implicit_skill_invocation_for_command( None } +pub(crate) fn detect_implicit_skill_script_invocation_for_command( + outcome: &SkillLoadOutcome, + command: &str, + workdir: &Path, +) -> Option { + let tokens = tokenize_command(command); + + detect_implicit_skill_script_invocation_for_tokens(outcome, tokens.as_slice(), workdir) +} + +pub(crate) fn detect_implicit_skill_script_invocation_for_tokens( + outcome: &SkillLoadOutcome, + command: &[String], + workdir: &Path, +) -> Option { + detect_skill_script_run(outcome, command, workdir) +} + +fn tokenize_command(command: &str) -> Vec { + shlex::split(command).unwrap_or_else(|| { + command + .split_whitespace() + .map(std::string::ToString::to_string) + .collect() + }) +} + +pub(crate) async fn ensure_skill_approval_for_command( + sess: &Session, + turn_context: &TurnContext, + item_id: &str, + command: &str, + workdir: &Path, +) -> bool { + if !turn_context.features.enabled(Feature::SkillApproval) { + return true; + } + + let workdir = normalize_path(workdir); + let Some(skill) = detect_implicit_skill_script_invocation_for_command( + turn_context.turn_skills.outcome.as_ref(), + command, + workdir.as_path(), + ) else { + return true; + }; + + let cache_key = SkillApprovalCacheKey { + skill_name: skill.name.clone(), + skill_path: skill.path.clone(), + skill_scope: skill.scope, + }; + let already_approved = { + let store = sess.services.tool_approvals.lock().await; + matches!( + store.get(&cache_key), + Some(ReviewDecision::ApprovedForSession) + ) + }; + if already_approved { + return true; + } + + let approved = sess + .request_skill_approval(turn_context, item_id.to_string(), skill.name) + .await + .is_some_and(|response| response.approved); + if !approved { + return false; + } + + let mut store = sess.services.tool_approvals.lock().await; + store.put(cache_key, ReviewDecision::ApprovedForSession); + true +} + pub(crate) async fn maybe_emit_implicit_skill_invocation( sess: &Session, turn_context: &TurnContext, @@ -115,15 +207,6 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation( ); } -fn tokenize_command(command: &str) -> Vec { - shlex::split(command).unwrap_or_else(|| { - command - .split_whitespace() - .map(std::string::ToString::to_string) - .collect() - }) -} - fn script_run_token(tokens: &[String]) -> Option<&str> { const RUNNERS: [&str; 10] = [ "python", "python3", "bash", "zsh", "sh", "node", "deno", "ruby", "perl", "pwsh", @@ -234,6 +317,7 @@ fn normalize_path(path: &Path) -> PathBuf { mod tests { use super::SkillLoadOutcome; use super::SkillMetadata; + use super::detect_implicit_skill_script_invocation_for_command; use super::detect_skill_doc_read; use super::detect_skill_script_run; use super::normalize_path; @@ -353,4 +437,50 @@ mod tests { Some("test-skill".to_string()) ); } + + #[test] + fn implicit_skill_script_invocation_matches_command() { + let skill_doc_path = PathBuf::from("/tmp/skill-test/SKILL.md"); + let scripts_dir = normalize_path(Path::new("/tmp/skill-test/scripts")); + let skill = test_skill_metadata(skill_doc_path); + let outcome = SkillLoadOutcome { + implicit_skills_by_scripts_dir: Arc::new(HashMap::from([(scripts_dir, skill)])), + implicit_skills_by_doc_path: Arc::new(HashMap::new()), + ..Default::default() + }; + + let found = detect_implicit_skill_script_invocation_for_command( + &outcome, + "python scripts/fetch_comments.py", + Path::new("/tmp/skill-test"), + ); + + assert_eq!( + found.map(|value| value.name), + Some("test-skill".to_string()) + ); + } + + #[test] + fn implicit_skill_script_invocation_ignores_doc_reads() { + let skill_doc_path = PathBuf::from("/tmp/skill-test/SKILL.md"); + let normalized_skill_doc_path = normalize_path(skill_doc_path.as_path()); + let skill = test_skill_metadata(skill_doc_path); + let outcome = SkillLoadOutcome { + implicit_skills_by_scripts_dir: Arc::new(HashMap::new()), + implicit_skills_by_doc_path: Arc::new(HashMap::from([( + normalized_skill_doc_path, + skill, + )])), + ..Default::default() + }; + + let found = detect_implicit_skill_script_invocation_for_command( + &outcome, + "cat SKILL.md", + Path::new("/tmp/skill-test"), + ); + + assert_eq!(found, None); + } } diff --git a/codex-rs/core/src/skills/mod.rs b/codex-rs/core/src/skills/mod.rs index 868a1d7de..6fccd71cc 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -14,7 +14,9 @@ pub(crate) use env_var_dependencies::resolve_skill_dependencies_for_turn; pub(crate) use injection::SkillInjections; pub(crate) use injection::build_skill_injections; pub(crate) use injection::collect_explicit_skill_mentions; +pub(crate) use invocation_utils::SKILL_APPROVAL_DECLINED_MESSAGE; pub(crate) use invocation_utils::build_implicit_skill_path_indexes; +pub(crate) use invocation_utils::ensure_skill_approval_for_command; pub(crate) use invocation_utils::maybe_emit_implicit_skill_invocation; pub use loader::load_skills; pub use manager::SkillsManager; diff --git a/codex-rs/core/src/tools/handlers/shell.rs b/codex-rs/core/src/tools/handlers/shell.rs index 79db57e2e..7a78ecb1c 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -14,6 +14,8 @@ use crate::function_tool::FunctionCallError; use crate::is_safe_command::is_known_safe_command; use crate::protocol::ExecCommandSource; use crate::shell::Shell; +use crate::skills::SKILL_APPROVAL_DECLINED_MESSAGE; +use crate::skills::ensure_skill_approval_for_command; use crate::skills::maybe_emit_implicit_skill_invocation; use crate::tools::context::ToolInvocation; use crate::tools::context::ToolOutput; @@ -66,6 +68,8 @@ impl ShellHandler { ) -> ExecParams { ExecParams { command: params.command.clone(), + original_command: shlex::try_join(params.command.iter().map(String::as_str)) + .unwrap_or_else(|_| params.command.join(" ")), cwd: turn_context.resolve_path(params.workdir.clone()), expiration: params.timeout_ms.into(), env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), @@ -116,6 +120,7 @@ impl ShellCommandHandler { Ok(ExecParams { command, + original_command: params.command.clone(), cwd: turn_context.resolve_path(params.workdir.clone()), expiration: params.timeout_ms.into(), env: create_env(&turn_context.shell_environment_policy, Some(thread_id)), @@ -265,13 +270,6 @@ impl ToolHandler for ShellCommandHandler { }; let params: ShellCommandToolCallParams = parse_arguments(&arguments)?; - maybe_emit_implicit_skill_invocation( - session.as_ref(), - turn.as_ref(), - ¶ms.command, - params.workdir.as_deref(), - ) - .await; let prefix_rule = params.prefix_rule.clone(); let exec_params = Self::to_exec_params( ¶ms, @@ -348,6 +346,28 @@ impl ShellHandler { "approval policy is {approval_policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {approval_policy:?}" ))); } + let original_command = exec_params.original_command.as_str(); + if !ensure_skill_approval_for_command( + session.as_ref(), + turn.as_ref(), + &call_id, + original_command, + exec_params.cwd.as_path(), + ) + .await + { + return Err(FunctionCallError::RespondToModel( + SKILL_APPROVAL_DECLINED_MESSAGE.to_string(), + )); + } + let workdir = exec_params.cwd.to_string_lossy().into_owned(); + maybe_emit_implicit_skill_invocation( + session.as_ref(), + turn.as_ref(), + original_command, + Some(workdir.as_str()), + ) + .await; // Intercept apply_patch if present. if let Some(output) = intercept_apply_patch( diff --git a/codex-rs/core/tests/suite/exec.rs b/codex-rs/core/tests/suite/exec.rs index 654660068..b6f78267b 100644 --- a/codex-rs/core/tests/suite/exec.rs +++ b/codex-rs/core/tests/suite/exec.rs @@ -33,6 +33,7 @@ async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result anyhow::Result<()> { - skip_if_no_network!(Ok(())); +fn write_skill_with_script(home: &Path, name: &str, script_body: &str) -> Result { + let skill_dir = home.join("skills").join(name); + let scripts_dir = skill_dir.join("scripts"); + fs::create_dir_all(&scripts_dir)?; + fs::write( + skill_dir.join("SKILL.md"), + format!("---\nname: {name}\ndescription: {name} skill\n---\n"), + )?; + let script_path = scripts_dir.join("run.py"); + fs::write(&script_path, script_body)?; + Ok(script_path) +} - let server = start_mock_server().await; - responses::mount_sse_once( - &server, - sse(vec![ - ev_response_created("resp-1"), - ev_assistant_message("msg-1", "approved"), - ev_completed("resp-1"), - ]), - ) - .await; +fn shell_command_arguments(command: &str) -> Result { + Ok(serde_json::to_string(&json!({ + "command": command, + "timeout_ms": 500, + }))?) +} - let builder = test_codex(); - let TestCodex { - codex, - cwd, - session_configured, - .. - } = builder - .with_config(|config| { - config.features.enable(Feature::SkillApproval); - }) - .build(&server) - .await?; +fn assistant_response(message: &str) -> String { + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-1", message), + ev_completed("resp-2"), + ]) +} - let turn_id = codex +fn command_for_script(script_path: &Path) -> Result { + let runner = if cfg!(windows) { "python" } else { "python3" }; + let script_path = script_path.to_string_lossy().into_owned(); + Ok(shlex::try_join([runner, script_path.as_str()])?) +} + +async fn submit_turn(test: &TestCodex, prompt: &str) -> Result<()> { + let session_model = test.session_configured.model.clone(); + test.codex .submit(Op::UserTurn { - items: vec![UserInput::Text { - text: "trigger skill approval test".into(), + items: vec![codex_protocol::user_input::UserInput::Text { + text: prompt.to_string(), text_elements: Vec::new(), }], final_output_json_schema: None, - cwd: cwd.path().to_path_buf(), - approval_policy: AskForApproval::Never, - sandbox_policy: SandboxPolicy::DangerFullAccess, - model: session_configured.model.clone(), + cwd: test.cwd_path().to_path_buf(), + approval_policy: codex_protocol::protocol::AskForApproval::Never, + sandbox_policy: codex_protocol::protocol::SandboxPolicy::DangerFullAccess, + model: session_model, effort: None, - summary: ReasoningSummary::Auto, + summary: codex_protocol::config_types::ReasoningSummary::Auto, collaboration_mode: None, personality: None, }) .await?; + Ok(()) +} - let request = wait_for_event_match(&codex, |event| match event { +async fn wait_for_turn_complete_without_skill_approval(test: &TestCodex) { + wait_for_event_match(test.codex.as_ref(), |event| match event { + EventMsg::SkillRequestApproval(request) => { + panic!("unexpected skill approval request: {request:?}"); + } + EventMsg::TurnComplete(_) => Some(()), + _ => None, + }) + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn skill_approval_event_round_trip_for_shell_command_skill_script_exec() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let tool_call_id = "shell-skill-call"; + let mut builder = test_codex() + .with_pre_build_hook(|home| { + write_skill_with_script(home, "demo", "print('shell skill approved')").unwrap(); + }) + .with_config(|config| { + config.features.enable(Feature::SkillApproval); + }); + let test = builder.build(&server).await?; + let script_path = test.codex_home_path().join("skills/demo/scripts/run.py"); + let command = command_for_script(&script_path)?; + let arguments = shell_command_arguments(&command)?; + let _mocks = + mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command") + .await; + + submit_turn(&test, "run the shell skill").await?; + + let request = wait_for_event_match(test.codex.as_ref(), |event| match event { EventMsg::SkillRequestApproval(request) => Some(request.clone()), _ => None, }) .await; - assert_eq!(request.item_id, turn_id); - assert_eq!(request.skill_name, "test-skill"); + assert_eq!(request.item_id, tool_call_id); + assert_eq!(request.skill_name, "demo"); - codex + test.codex .submit(Op::SkillApproval { id: request.item_id, response: SkillApprovalResponse { approved: true }, }) .await?; - wait_for_event(&codex, |event| matches!(event, EventMsg::TurnComplete(_))).await; + wait_for_event(test.codex.as_ref(), |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn skill_approval_not_emitted_without_skill_script_exec() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let tool_call_id = "non-skill-call"; + let mut builder = test_codex().with_config(|config| { + config.features.enable(Feature::SkillApproval); + }); + let test = builder.build(&server).await?; + let arguments = shell_command_arguments("echo no-skill")?; + let _mocks = + mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command") + .await; + + submit_turn(&test, "run a plain command").await?; + wait_for_turn_complete_without_skill_approval(&test).await; + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn skill_approval_decline_blocks_execution() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let tool_call_id = "decline-call"; + let marker_name = "declined-marker.txt"; + let mut builder = test_codex() + .with_pre_build_hook(move |home| { + let marker_path = home.join(marker_name); + let marker_path = marker_path.to_string_lossy(); + let script_body = format!( + "from pathlib import Path\nPath({marker_path:?}).write_text('ran')\nprint('ran')\n" + ); + write_skill_with_script(home, "demo", &script_body).unwrap(); + }) + .with_config(|config| { + config.features.enable(Feature::SkillApproval); + }); + let test = builder.build(&server).await?; + let script_path = test.codex_home_path().join("skills/demo/scripts/run.py"); + let command = command_for_script(&script_path)?; + let arguments = shell_command_arguments(&command)?; + let mocks = + mount_function_call_agent_response(&server, tool_call_id, &arguments, "shell_command") + .await; + + submit_turn(&test, "run the skill").await?; + + let request = wait_for_event_match(test.codex.as_ref(), |event| match event { + EventMsg::SkillRequestApproval(request) => Some(request.clone()), + _ => None, + }) + .await; + assert_eq!(request.item_id, tool_call_id); + + test.codex + .submit(Op::SkillApproval { + id: request.item_id, + response: SkillApprovalResponse { approved: false }, + }) + .await?; + + wait_for_event(test.codex.as_ref(), |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let marker_path = test.codex_home_path().join(marker_name); + assert!( + !marker_path.exists(), + "declined skill approval should block script execution" + ); + + let call_output = mocks + .completion + .single_request() + .function_call_output(tool_call_id); + assert_eq!( + call_output["output"].as_str(), + Some("This script is part of the skill and the user declined the skill usage"), + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn skill_approval_cache_is_per_skill() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let first_call_id = "skill-a-1"; + let second_call_id = "skill-a-2"; + let third_call_id = "skill-b-1"; + let mut builder = test_codex() + .with_pre_build_hook(|home| { + write_skill_with_script(home, "alpha", "print('alpha')").unwrap(); + write_skill_with_script(home, "beta", "print('beta')").unwrap(); + }) + .with_config(|config| { + config.features.enable(Feature::SkillApproval); + }); + let test = builder.build(&server).await?; + let alpha_command = + command_for_script(&test.codex_home_path().join("skills/alpha/scripts/run.py"))?; + let beta_command = + command_for_script(&test.codex_home_path().join("skills/beta/scripts/run.py"))?; + let first_alpha_arguments = shell_command_arguments(&alpha_command)?; + let second_alpha_arguments = shell_command_arguments(&alpha_command)?; + let beta_arguments = shell_command_arguments(&beta_command)?; + + mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(first_call_id, "shell_command", &first_alpha_arguments), + ev_completed("resp-1"), + ]), + assistant_response("alpha-1"), + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(second_call_id, "shell_command", &second_alpha_arguments), + ev_completed("resp-1"), + ]), + assistant_response("alpha-2"), + sse(vec![ + ev_response_created("resp-1"), + ev_function_call(third_call_id, "shell_command", &beta_arguments), + ev_completed("resp-1"), + ]), + assistant_response("beta-1"), + ], + ) + .await; + + submit_turn(&test, "run alpha").await?; + let first_request = wait_for_event_match(test.codex.as_ref(), |event| match event { + EventMsg::SkillRequestApproval(request) => Some(request.clone()), + _ => None, + }) + .await; + assert_eq!(first_request.item_id, first_call_id); + assert_eq!(first_request.skill_name, "alpha"); + test.codex + .submit(Op::SkillApproval { + id: first_request.item_id, + response: SkillApprovalResponse { approved: true }, + }) + .await?; + wait_for_event(test.codex.as_ref(), |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + submit_turn(&test, "run alpha again").await?; + wait_for_turn_complete_without_skill_approval(&test).await; + + submit_turn(&test, "run beta").await?; + let third_request = wait_for_event_match(test.codex.as_ref(), |event| match event { + EventMsg::SkillRequestApproval(request) => Some(request.clone()), + _ => None, + }) + .await; + assert_eq!(third_request.item_id, third_call_id); + assert_eq!(third_request.skill_name, "beta"); + test.codex + .submit(Op::SkillApproval { + id: third_request.item_id, + response: SkillApprovalResponse { approved: true }, + }) + .await?; + wait_for_event(test.codex.as_ref(), |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; Ok(()) } diff --git a/codex-rs/linux-sandbox/tests/suite/landlock.rs b/codex-rs/linux-sandbox/tests/suite/landlock.rs index 362fcaf35..b9c0d6c8b 100644 --- a/codex-rs/linux-sandbox/tests/suite/landlock.rs +++ b/codex-rs/linux-sandbox/tests/suite/landlock.rs @@ -73,6 +73,7 @@ async fn run_cmd_result_with_writable_roots( let sandbox_cwd = cwd.clone(); let params = ExecParams { command: cmd.iter().copied().map(str::to_owned).collect(), + original_command: cmd.iter().copied().map(str::to_owned).collect(), cwd, expiration: timeout_ms.into(), env: create_env_from_core_vars(), @@ -315,6 +316,7 @@ async fn assert_network_blocked(cmd: &[&str]) { let sandbox_cwd = cwd.clone(); let params = ExecParams { command: cmd.iter().copied().map(str::to_owned).collect(), + original_command: cmd.iter().copied().map(str::to_owned).collect(), cwd, // Give the tool a generous 2-second timeout so even slow DNS timeouts // do not stall the suite.