From b6d20748e0b9d240ce7edd10ab87199e35390131 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Wed, 25 Feb 2026 14:49:53 -0800 Subject: [PATCH] Revert "Ensure shell command skills trigger approval (#12697)" (#12721) This reverts commit daf0f03ac80182e500f8731f42ed50daae9400ee. # External (non-OpenAI) Pull Request Requirements Before opening this Pull Request, please read the dedicated "Contributing" markdown file or your PR may be closed: https://github.com/openai/codex/blob/main/docs/contributing.md If your PR conforms to our contribution guidelines, replace this text with a detailed and high quality description of your changes. Include a link to a bug report or enhancement request. --- .../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 | 298 +----------------- .../linux-sandbox/tests/suite/landlock.rs | 2 - 10 files changed, 83 insertions(+), 556 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 520c9475c..f6747b61d 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -1813,10 +1813,8 @@ impl CodexMessageProcessor { None => None, }; let windows_sandbox_level = WindowsSandboxLevel::from_config(&self.config); - let command = params.command; let exec_params = ExecParams { - original_command: command.join(" "), - command, + command: params.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 a86e45976..945a545a8 100644 --- a/codex-rs/app-server/tests/suite/v2/skill_approval.rs +++ b/codex-rs/app-server/tests/suite/v2/skill_approval.rs @@ -3,7 +3,6 @@ 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; @@ -12,72 +11,19 @@ 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_on_shell_command_skill_script_exec() -> Result<()> { +async fn skill_request_approval_round_trip() -> Result<()> { let codex_home = tempfile::TempDir::new()?; - 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 server = + create_mock_responses_server_sequence(vec![create_final_assistant_message_sse_response( + "done", + )?]) + .await; + create_config_toml(codex_home.path(), &server.uri())?; let mut mcp = McpProcess::new(codex_home.path()).await?; timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; @@ -111,7 +57,7 @@ async fn skill_request_approval_round_trip_on_shell_command_skill_script_exec() mcp.read_stream_until_response_message(RequestId::Integer(turn_start_id)), ) .await??; - let TurnStartResponse { .. } = to_response::(turn_start_resp)?; + let TurnStartResponse { turn, .. } = to_response(turn_start_resp)?; let server_req = timeout( DEFAULT_READ_TIMEOUT, @@ -122,8 +68,8 @@ async fn skill_request_approval_round_trip_on_shell_command_skill_script_exec() panic!("expected SkillRequestApproval request, got: {server_req:?}"); }; - assert_eq!(params.item_id, tool_call_id); - assert_eq!(params.skill_name, "demo"); + assert_eq!(params.item_id, turn.id); + assert_eq!(params.skill_name, "test-skill"); mcp.send_response(request_id, serde_json::json!({ "decision": "approve" })) .await?; @@ -136,3 +82,28 @@ async fn skill_request_approval_round_trip_on_shell_command_skill_script_exec() 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 5a555d6a7..3d27faf9b 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -4818,6 +4818,15 @@ 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 @@ -9416,23 +9425,20 @@ 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: command.clone(), - original_command: shlex::try_join(command.iter().map(String::as_str)) - .unwrap_or_else(|_| command.join(" ")), + 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(), + ] + }, cwd: turn_context.cwd.clone(), expiration: timeout_ms.into(), env: HashMap::new(), @@ -9446,7 +9452,6 @@ 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 1d1169303..288ee7f21 100644 --- a/codex-rs/core/src/exec.rs +++ b/codex-rs/core/src/exec.rs @@ -62,7 +62,6 @@ 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, @@ -181,7 +180,6 @@ pub async fn process_exec_tool_call( let ExecParams { command, - original_command: _, cwd, mut env, expiration, @@ -251,8 +249,6 @@ 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, @@ -1125,8 +1121,6 @@ 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(), @@ -1180,8 +1174,6 @@ 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 5e6a851db..d2831c4df 100644 --- a/codex-rs/core/src/skills/invocation_utils.rs +++ b/codex-rs/core/src/skills/invocation_utils.rs @@ -7,21 +7,8 @@ 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, @@ -54,11 +41,8 @@ fn detect_implicit_skill_invocation_for_command( let workdir = normalize_path(workdir.as_path()); let tokens = tokenize_command(command); - if let Some(candidate) = detect_implicit_skill_script_invocation_for_tokens( - outcome, - tokens.as_slice(), - workdir.as_path(), - ) { + if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), workdir.as_path()) + { return Some(candidate); } @@ -69,82 +53,6 @@ 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_to_skills_md.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, @@ -207,6 +115,15 @@ 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", @@ -317,7 +234,6 @@ 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; @@ -438,50 +354,4 @@ 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 6fccd71cc..868a1d7de 100644 --- a/codex-rs/core/src/skills/mod.rs +++ b/codex-rs/core/src/skills/mod.rs @@ -14,9 +14,7 @@ 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 95dbe05e5..9a1ff0052 100644 --- a/codex-rs/core/src/tools/handlers/shell.rs +++ b/codex-rs/core/src/tools/handlers/shell.rs @@ -14,8 +14,6 @@ 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; @@ -68,8 +66,6 @@ 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)), @@ -120,7 +116,6 @@ 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)), @@ -272,6 +267,13 @@ 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,28 +350,6 @@ 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 b6f78267b..654660068 100644 --- a/codex-rs/core/tests/suite/exec.rs +++ b/codex-rs/core/tests/suite/exec.rs @@ -33,7 +33,6 @@ async fn run_test_cmd(tmp: TempDir, cmd: Vec<&str>) -> Result 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!( - r#"--- -name: {name} -description: {name} skill ---- -"# - ), - )?; - let script_path = scripts_dir.join("run.py"); - fs::write(&script_path, script_body)?; - Ok(script_path) -} - -#[cfg(unix)] fn write_skill_metadata(home: &Path, name: &str, contents: &str) -> Result<()> { let metadata_dir = home.join("skills").join(name).join("agents"); fs::create_dir_all(&metadata_dir)?; @@ -64,40 +38,15 @@ fn shell_command_arguments(command: &str) -> Result { }))?) } -fn assistant_response(message: &str) -> String { - sse(vec![ - ev_response_created("resp-2"), - ev_assistant_message("msg-1", message), - ev_completed("resp-2"), - ]) -} - -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<()> { - submit_turn_with_policies( - test, - prompt, - AskForApproval::Never, - SandboxPolicy::DangerFullAccess, - ) - .await -} - async fn submit_turn_with_policies( test: &TestCodex, prompt: &str, approval_policy: AskForApproval, sandbox_policy: SandboxPolicy, ) -> Result<()> { - let session_model = test.session_configured.model.clone(); test.codex .submit(Op::UserTurn { - items: vec![codex_protocol::user_input::UserInput::Text { + items: vec![UserInput::Text { text: prompt.to_string(), text_elements: Vec::new(), }], @@ -105,9 +54,9 @@ async fn submit_turn_with_policies( cwd: test.cwd_path().to_path_buf(), approval_policy, sandbox_policy, - model: session_model, + model: test.session_configured.model.clone(), effort: None, - summary: codex_protocol::config_types::ReasoningSummary::Auto, + summary: ReasoningSummary::Auto, collaboration_mode: None, personality: None, }) @@ -126,7 +75,6 @@ async fn wait_for_turn_complete_without_skill_approval(test: &TestCodex) { .await; } -#[cfg(unix)] fn write_skill_with_shell_script(home: &Path, name: &str, script_name: &str) -> Result { use std::os::unix::fs::PermissionsExt; @@ -158,7 +106,6 @@ echo 'zsh-fork-stderr' >&2 Ok(script_path) } -#[cfg(unix)] fn find_test_zsh_path() -> Result> { use core_test_support::fetch_dotslash_file; @@ -181,7 +128,6 @@ fn find_test_zsh_path() -> Result> { } } -#[cfg(unix)] fn supports_exec_wrapper_intercept(zsh_path: &Path) -> bool { let status = std::process::Command::new(zsh_path) .arg("-fc") @@ -194,236 +140,6 @@ fn supports_exec_wrapper_intercept(zsh_path: &Path) -> bool { } } -#[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, tool_call_id); - assert_eq!(request.skill_name, "demo"); - - test.codex - .submit(Op::SkillApproval { - id: request.item_id, - response: SkillApprovalResponse { approved: true }, - }) - .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!( - r#"from pathlib import Path -Path({marker_path:?}).write_text('ran') -print('ran') -"# - ); - 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(()) -} - #[cfg(unix)] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn shell_zsh_fork_prompts_for_skill_script_execution() -> Result<()> { diff --git a/codex-rs/linux-sandbox/tests/suite/landlock.rs b/codex-rs/linux-sandbox/tests/suite/landlock.rs index b9c0d6c8b..362fcaf35 100644 --- a/codex-rs/linux-sandbox/tests/suite/landlock.rs +++ b/codex-rs/linux-sandbox/tests/suite/landlock.rs @@ -73,7 +73,6 @@ 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(), @@ -316,7 +315,6 @@ 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.