diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 7137bf4d3a..ee22adaa4c 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -941,6 +941,7 @@ impl TurnContext { sandbox_policy: self.sandbox_policy.get(), windows_sandbox_level: self.windows_sandbox_level, }) + .with_has_attached_executor(self.environment.has_attached_executor()) .with_unified_exec_shell_mode(self.tools_config.unified_exec_shell_mode.clone()) .with_web_search_config(self.tools_config.web_search_config.clone()) .with_allow_login_shell(self.tools_config.allow_login_shell) @@ -1395,6 +1396,7 @@ impl Session { sandbox_policy: session_configuration.sandbox_policy.get(), windows_sandbox_level: session_configuration.windows_sandbox_level, }) + .with_has_attached_executor(environment.has_attached_executor()) .with_unified_exec_shell_mode_for_session( user_shell, shell_zsh_path, @@ -5472,6 +5474,7 @@ async fn spawn_review_thread( sess.services.shell_zsh_path.as_ref(), sess.services.main_execve_wrapper_exe.as_ref(), ) + .with_has_attached_executor(parent_turn_context.environment.has_attached_executor()) .with_web_search_config(/*web_search_config*/ None) .with_allow_login_shell(config.permissions.allow_login_shell) .with_agent_roles(config.agent_roles.clone()); diff --git a/codex-rs/core/src/codex_delegate.rs b/codex-rs/core/src/codex_delegate.rs index 3f1508cca1..168db3d89a 100644 --- a/codex-rs/core/src/codex_delegate.rs +++ b/codex-rs/core/src/codex_delegate.rs @@ -77,8 +77,8 @@ pub(crate) async fn run_codex_thread_interactive( config, auth_manager, models_manager, - environment_manager: Arc::new(EnvironmentManager::new( - parent_ctx.environment.exec_server_url().map(str::to_owned), + environment_manager: Arc::new(EnvironmentManager::from_environment( + parent_ctx.environment.as_ref(), )), skills_manager: Arc::clone(&parent_session.services.skills_manager), plugins_manager: Arc::clone(&parent_session.services.plugins_manager), diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index d0614fc35b..f9ccc5d98f 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -171,6 +171,7 @@ pub(crate) struct ToolsConfig { pub experimental_supported_tools: Vec, pub agent_jobs_tools: bool, pub agent_jobs_worker_tools: bool, + pub has_attached_executor: bool, } pub(crate) struct ToolsConfigParams<'a> { @@ -305,9 +306,15 @@ impl ToolsConfig { experimental_supported_tools: model_info.experimental_supported_tools.clone(), agent_jobs_tools: include_agent_jobs, agent_jobs_worker_tools, + has_attached_executor: true, } } + pub fn with_has_attached_executor(mut self, has_attached_executor: bool) -> Self { + self.has_attached_executor = has_attached_executor; + self + } + pub fn with_agent_roles(mut self, agent_roles: BTreeMap) -> Self { self.agent_roles = agent_roles; self @@ -493,66 +500,68 @@ pub(crate) fn build_specs_with_discoverable_tools( builder.register_handler(WAIT_TOOL_NAME, code_mode_wait_handler); } - match &config.shell_type { - ConfigShellToolType::Default => { - push_tool_spec( - &mut builder, - create_shell_tool(ShellToolOptions { - exec_permission_approvals_enabled, - }), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); + if config.has_attached_executor { + match &config.shell_type { + ConfigShellToolType::Default => { + push_tool_spec( + &mut builder, + create_shell_tool(ShellToolOptions { + exec_permission_approvals_enabled, + }), + /*supports_parallel_tool_calls*/ true, + config.code_mode_enabled, + ); + } + ConfigShellToolType::Local => { + push_tool_spec( + &mut builder, + ToolSpec::LocalShell {}, + /*supports_parallel_tool_calls*/ true, + config.code_mode_enabled, + ); + } + ConfigShellToolType::UnifiedExec => { + push_tool_spec( + &mut builder, + create_exec_command_tool(CommandToolOptions { + allow_login_shell: config.allow_login_shell, + exec_permission_approvals_enabled, + }), + /*supports_parallel_tool_calls*/ true, + config.code_mode_enabled, + ); + push_tool_spec( + &mut builder, + create_write_stdin_tool(), + /*supports_parallel_tool_calls*/ false, + config.code_mode_enabled, + ); + builder.register_handler("exec_command", unified_exec_handler.clone()); + builder.register_handler("write_stdin", unified_exec_handler); + } + ConfigShellToolType::Disabled => { + // Do nothing. + } + ConfigShellToolType::ShellCommand => { + push_tool_spec( + &mut builder, + create_shell_command_tool(CommandToolOptions { + allow_login_shell: config.allow_login_shell, + exec_permission_approvals_enabled, + }), + /*supports_parallel_tool_calls*/ true, + config.code_mode_enabled, + ); + } } - ConfigShellToolType::Local => { - push_tool_spec( - &mut builder, - ToolSpec::LocalShell {}, - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - } - ConfigShellToolType::UnifiedExec => { - push_tool_spec( - &mut builder, - create_exec_command_tool(CommandToolOptions { - allow_login_shell: config.allow_login_shell, - exec_permission_approvals_enabled, - }), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - push_tool_spec( - &mut builder, - create_write_stdin_tool(), - /*supports_parallel_tool_calls*/ false, - config.code_mode_enabled, - ); - builder.register_handler("exec_command", unified_exec_handler.clone()); - builder.register_handler("write_stdin", unified_exec_handler); - } - ConfigShellToolType::Disabled => { - // Do nothing. - } - ConfigShellToolType::ShellCommand => { - push_tool_spec( - &mut builder, - create_shell_command_tool(CommandToolOptions { - allow_login_shell: config.allow_login_shell, - exec_permission_approvals_enabled, - }), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - } - } - if config.shell_type != ConfigShellToolType::Disabled { - // Always register shell aliases so older prompts remain compatible. - builder.register_handler("shell", shell_handler.clone()); - builder.register_handler("container.exec", shell_handler.clone()); - builder.register_handler("local_shell", shell_handler); - builder.register_handler("shell_command", shell_command_handler); + if config.shell_type != ConfigShellToolType::Disabled { + // Always register shell aliases so older prompts remain compatible. + builder.register_handler("shell", shell_handler.clone()); + builder.register_handler("container.exec", shell_handler.clone()); + builder.register_handler("local_shell", shell_handler); + builder.register_handler("shell_command", shell_command_handler); + } } if mcp_tools.is_some() { @@ -587,7 +596,7 @@ pub(crate) fn build_specs_with_discoverable_tools( ); builder.register_handler("update_plan", plan_handler); - if config.js_repl_enabled { + if config.has_attached_executor && config.js_repl_enabled { push_tool_spec( &mut builder, create_js_repl_tool(), @@ -661,7 +670,9 @@ pub(crate) fn build_specs_with_discoverable_tools( builder.register_handler(TOOL_SUGGEST_TOOL_NAME, tool_suggest_handler); } - if let Some(apply_patch_tool_type) = &config.apply_patch_tool_type { + if config.has_attached_executor + && let Some(apply_patch_tool_type) = &config.apply_patch_tool_type + { match apply_patch_tool_type { ApplyPatchToolType::Freeform => { push_tool_spec( @@ -683,10 +694,11 @@ pub(crate) fn build_specs_with_discoverable_tools( builder.register_handler("apply_patch", apply_patch_handler); } - if config - .experimental_supported_tools - .iter() - .any(|tool| tool == "list_dir") + if config.has_attached_executor + && config + .experimental_supported_tools + .iter() + .any(|tool| tool == "list_dir") { let list_dir_handler = Arc::new(ListDirHandler); push_tool_spec( @@ -763,15 +775,17 @@ pub(crate) fn build_specs_with_discoverable_tools( ); } - push_tool_spec( - &mut builder, - create_view_image_tool(ViewImageToolOptions { - can_request_original_image_detail: config.can_request_original_image_detail, - }), - /*supports_parallel_tool_calls*/ true, - config.code_mode_enabled, - ); - builder.register_handler("view_image", view_image_handler); + if config.has_attached_executor { + push_tool_spec( + &mut builder, + create_view_image_tool(ViewImageToolOptions { + can_request_original_image_detail: config.can_request_original_image_detail, + }), + /*supports_parallel_tool_calls*/ true, + config.code_mode_enabled, + ); + builder.register_handler("view_image", view_image_handler); + } if config.collab_tools { if config.multi_agent_v2 { diff --git a/codex-rs/core/src/tools/spec_tests.rs b/codex-rs/core/src/tools/spec_tests.rs index e2dd827d6f..eb450c5e1c 100644 --- a/codex-rs/core/src/tools/spec_tests.rs +++ b/codex-rs/core/src/tools/spec_tests.rs @@ -987,6 +987,53 @@ fn js_repl_enabled_adds_tools() { assert_contains_tool_names(&tools, &["js_repl", "js_repl_reset"]); } +#[test] +fn no_attached_executor_hides_executor_backed_tools() { + let model_info = model_info_from_models_json("gpt-5-codex"); + let mut features = Features::with_defaults(); + features.enable(Feature::UnifiedExec); + features.enable(Feature::JsRepl); + let available_models = Vec::new(); + let mut tools_config = ToolsConfig::new(&ToolsConfigParams { + model_info: &model_info, + available_models: &available_models, + features: &features, + web_search_mode: Some(WebSearchMode::Live), + session_source: SessionSource::Cli, + sandbox_policy: &SandboxPolicy::DangerFullAccess, + windows_sandbox_level: WindowsSandboxLevel::Disabled, + }) + .with_has_attached_executor(false); + tools_config + .experimental_supported_tools + .push("list_dir".to_string()); + + let (tools, _) = build_specs( + &tools_config, + Some(std::collections::HashMap::new()), + /*app_tools*/ None, + &[], + ) + .build(); + + if let Some(shell_tool) = shell_tool_name(&tools_config) { + assert_lacks_tool_name(&tools, shell_tool); + } + for absent in [ + "exec_command", + "write_stdin", + "apply_patch", + "list_dir", + VIEW_IMAGE_TOOL_NAME, + "js_repl", + "js_repl_reset", + ] { + assert_lacks_tool_name(&tools, absent); + } + + assert_contains_tool_names(&tools, &["update_plan", "request_user_input", "web_search"]); +} + #[test] fn image_generation_tools_require_feature_and_supported_model() { let config = test_config(); diff --git a/codex-rs/core/tests/common/test_codex.rs b/codex-rs/core/tests/common/test_codex.rs index 3b910838bc..2e37df237c 100644 --- a/codex-rs/core/tests/common/test_codex.rs +++ b/codex-rs/core/tests/common/test_codex.rs @@ -478,9 +478,9 @@ impl TestCodexBuilder { test_env: TestEnv, ) -> anyhow::Result { let auth = self.auth.clone(); - let environment_manager = Arc::new(codex_exec_server::EnvironmentManager::new( - test_env.exec_server_url().map(str::to_owned), - )); + let environment_manager = Arc::new( + codex_exec_server::EnvironmentManager::from_environment(test_env.environment()), + ); let thread_manager = if config.model_catalog.is_some() { ThreadManager::new( &config, diff --git a/codex-rs/exec-server/src/environment.rs b/codex-rs/exec-server/src/environment.rs index 0518612c38..4882e9d487 100644 --- a/codex-rs/exec-server/src/environment.rs +++ b/codex-rs/exec-server/src/environment.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use async_trait::async_trait; use tokio::sync::OnceCell; use crate::ExecServerClient; @@ -9,6 +10,8 @@ use crate::file_system::ExecutorFileSystem; use crate::local_file_system::LocalFileSystem; use crate::local_process::LocalProcess; use crate::process::ExecBackend; +use crate::process::StartedExecProcess; +use crate::protocol::ExecParams; use crate::remote_file_system::RemoteFileSystem; use crate::remote_process::RemoteProcess; @@ -20,14 +23,21 @@ pub trait ExecutorEnvironment: Send + Sync { #[derive(Debug, Default)] pub struct EnvironmentManager { - exec_server_url: Option, + executor_mode: ExecutorMode, current_environment: OnceCell>, } impl EnvironmentManager { pub fn new(exec_server_url: Option) -> Self { Self { - exec_server_url: normalize_exec_server_url(exec_server_url), + executor_mode: parse_executor_mode(exec_server_url), + current_environment: OnceCell::new(), + } + } + + pub fn from_environment(environment: &Environment) -> Self { + Self { + executor_mode: environment.executor_mode.clone(), current_environment: OnceCell::new(), } } @@ -37,14 +47,14 @@ impl EnvironmentManager { } pub fn exec_server_url(&self) -> Option<&str> { - self.exec_server_url.as_deref() + self.executor_mode.remote_exec_server_url() } pub async fn current(&self) -> Result, ExecServerError> { self.current_environment .get_or_try_init(|| async { Ok(Arc::new( - Environment::create(self.exec_server_url.clone()).await?, + Environment::create_with_mode(self.executor_mode.clone()).await?, )) }) .await @@ -52,9 +62,32 @@ impl EnvironmentManager { } } +#[derive(Clone, Debug, Default, Eq, PartialEq)] +enum ExecutorMode { + #[default] + LocalExecutor, + RemoteExecutor { + url: String, + }, + NoExecutor, +} + +impl ExecutorMode { + fn remote_exec_server_url(&self) -> Option<&str> { + match self { + Self::RemoteExecutor { url } => Some(url.as_str()), + Self::LocalExecutor | Self::NoExecutor => None, + } + } + + fn has_attached_executor(&self) -> bool { + !matches!(self, Self::NoExecutor) + } +} + #[derive(Clone)] pub struct Environment { - exec_server_url: Option, + executor_mode: ExecutorMode, remote_exec_server_client: Option, exec_backend: Arc, } @@ -70,7 +103,7 @@ impl Default for Environment { } Self { - exec_server_url: None, + executor_mode: ExecutorMode::LocalExecutor, remote_exec_server_client: None, exec_backend: Arc::new(local_process), } @@ -80,18 +113,21 @@ impl Default for Environment { impl std::fmt::Debug for Environment { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("Environment") - .field("exec_server_url", &self.exec_server_url) + .field("executor_mode", &self.executor_mode) .finish_non_exhaustive() } } impl Environment { pub async fn create(exec_server_url: Option) -> Result { - let exec_server_url = normalize_exec_server_url(exec_server_url); - let remote_exec_server_client = if let Some(url) = &exec_server_url { + Self::create_with_mode(parse_executor_mode(exec_server_url)).await + } + + async fn create_with_mode(executor_mode: ExecutorMode) -> Result { + let remote_exec_server_client = if let Some(url) = executor_mode.remote_exec_server_url() { Some( ExecServerClient::connect_websocket(RemoteExecServerConnectArgs { - websocket_url: url.clone(), + websocket_url: url.to_string(), client_name: "codex-environment".to_string(), connect_timeout: std::time::Duration::from_secs(5), initialize_timeout: std::time::Duration::from_secs(5), @@ -105,6 +141,8 @@ impl Environment { let exec_backend: Arc = if let Some(client) = remote_exec_server_client.clone() { Arc::new(RemoteProcess::new(client)) + } else if matches!(executor_mode, ExecutorMode::NoExecutor) { + Arc::new(NoAttachedExecutorBackend) } else { let local_process = LocalProcess::default(); local_process @@ -117,14 +155,18 @@ impl Environment { }; Ok(Self { - exec_server_url, + executor_mode, remote_exec_server_client, exec_backend, }) } pub fn exec_server_url(&self) -> Option<&str> { - self.exec_server_url.as_deref() + self.executor_mode.remote_exec_server_url() + } + + pub fn has_attached_executor(&self) -> bool { + self.executor_mode.has_attached_executor() } pub fn get_exec_backend(&self) -> Arc { @@ -140,11 +182,26 @@ impl Environment { } } -fn normalize_exec_server_url(exec_server_url: Option) -> Option { - exec_server_url.and_then(|url| { - let url = url.trim(); - (!url.is_empty()).then(|| url.to_string()) - }) +#[derive(Clone, Default)] +struct NoAttachedExecutorBackend; + +#[async_trait] +impl ExecBackend for NoAttachedExecutorBackend { + async fn start(&self, _params: ExecParams) -> Result { + Err(ExecServerError::Protocol( + "no attached executor is configured for this session".to_string(), + )) + } +} + +fn parse_executor_mode(exec_server_url: Option) -> ExecutorMode { + match exec_server_url.as_deref().map(str::trim) { + None | Some("") => ExecutorMode::LocalExecutor, + Some(url) if url.eq_ignore_ascii_case("none") => ExecutorMode::NoExecutor, + Some(url) => ExecutorMode::RemoteExecutor { + url: url.to_string(), + }, + } } impl ExecutorEnvironment for Environment { @@ -169,6 +226,8 @@ mod tests { .expect("create environment"); assert_eq!(environment.exec_server_url(), None); + assert!(environment.has_attached_executor()); + assert_eq!(environment.executor_mode, ExecutorMode::LocalExecutor); assert!(environment.remote_exec_server_client.is_none()); } @@ -176,7 +235,37 @@ mod tests { fn environment_manager_normalizes_empty_url() { let manager = EnvironmentManager::new(Some(String::new())); - assert_eq!(manager.exec_server_url(), None); + assert_eq!(manager.executor_mode, ExecutorMode::LocalExecutor); + } + + #[test] + fn environment_manager_preserves_no_executor_setting() { + let manager = EnvironmentManager::new(Some("none".to_string())); + + assert_eq!(manager.executor_mode, ExecutorMode::NoExecutor); + } + + #[test] + fn parse_executor_mode_preserves_no_executor_semantics() { + assert_eq!(parse_executor_mode(None), ExecutorMode::LocalExecutor); + assert_eq!( + parse_executor_mode(Some(String::new())), + ExecutorMode::LocalExecutor + ); + assert_eq!( + parse_executor_mode(Some("none".to_string())), + ExecutorMode::NoExecutor + ); + assert_eq!( + parse_executor_mode(Some("NONE".to_string())), + ExecutorMode::NoExecutor + ); + assert_eq!( + parse_executor_mode(Some("ws://localhost:1234".to_string())), + ExecutorMode::RemoteExecutor { + url: "ws://localhost:1234".to_string(), + } + ); } #[tokio::test] @@ -208,4 +297,41 @@ mod tests { assert_eq!(response.process.process_id().as_str(), "default-env-proc"); } + + #[tokio::test] + async fn no_executor_environment_disables_attached_executor() { + let environment = Environment::create(Some("none".to_string())) + .await + .expect("create environment"); + + assert_eq!(environment.exec_server_url(), None); + assert!(!environment.has_attached_executor()); + assert_eq!(environment.executor_mode, ExecutorMode::NoExecutor); + assert!(environment.remote_exec_server_client.is_none()); + } + + #[tokio::test] + async fn no_executor_environment_rejects_exec_start() { + let environment = Environment::create(Some("none".to_string())) + .await + .expect("create environment"); + + let err = environment + .get_exec_backend() + .start(crate::ExecParams { + process_id: ProcessId::from("no-executor-proc"), + argv: vec!["true".to_string()], + cwd: std::env::current_dir().expect("read current dir"), + env: Default::default(), + tty: false, + arg0: None, + }) + .await + .expect_err("no-executor backend should reject starts"); + + assert_eq!( + err.to_string(), + "exec-server protocol error: no attached executor is configured for this session" + ); + } }