From 7b20db942a33cb17925d76305d515d986a7b02c8 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 24 Aug 2025 22:43:42 -0700 Subject: [PATCH 1/2] fix: build is broken on main; introduce ToolsConfigParams to help fix (#2663) `ToolsConfig::new()` taking a large number of boolean params was hard to manage and it finally bit us (see https://github.com/openai/codex/pull/2660). This changes `ToolsConfig::new()` so that it takes a struct (and also reduces the visibility of some members, where possible). --- codex-rs/core/src/codex.rs | 52 +++++---- codex-rs/core/src/openai_tools.rs | 182 ++++++++++++++++-------------- 2 files changed, 124 insertions(+), 110 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index e175f55094..9e08ded8eb 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -64,6 +64,7 @@ use crate::mcp_tool_call::handle_mcp_tool_call; use crate::model_family::find_family_for_model; use crate::openai_tools::ApplyPatchToolArgs; use crate::openai_tools::ToolsConfig; +use crate::openai_tools::ToolsConfigParams; use crate::openai_tools::get_openai_tools; use crate::parse_command::parse_command; use crate::plan_tool::handle_update_plan; @@ -506,15 +507,15 @@ impl Session { ); let turn_context = TurnContext { client, - tools_config: ToolsConfig::new( - &config.model_family, + tools_config: ToolsConfig::new(&ToolsConfigParams { + model_family: &config.model_family, approval_policy, - sandbox_policy.clone(), - config.include_plan_tool, - config.include_apply_patch_tool, - config.tools_web_search_request, - config.use_experimental_streamable_shell_tool, - ), + sandbox_policy: sandbox_policy.clone(), + include_plan_tool: config.include_plan_tool, + include_apply_patch_tool: config.include_apply_patch_tool, + include_web_search_request: config.tools_web_search_request, + use_streamable_shell_tool: config.use_experimental_streamable_shell_tool, + }), user_instructions, base_instructions, approval_policy, @@ -1092,15 +1093,15 @@ async fn submission_loop( .unwrap_or(prev.sandbox_policy.clone()); let new_cwd = cwd.clone().unwrap_or_else(|| prev.cwd.clone()); - let tools_config = ToolsConfig::new( - &effective_family, - new_approval_policy, - new_sandbox_policy.clone(), - config.include_plan_tool, - config.include_apply_patch_tool, - config.tools_web_search_request, - config.use_experimental_streamable_shell_tool, - ); + let tools_config = ToolsConfig::new(&ToolsConfigParams { + model_family: &effective_family, + approval_policy: new_approval_policy, + sandbox_policy: new_sandbox_policy.clone(), + include_plan_tool: config.include_plan_tool, + include_apply_patch_tool: config.include_apply_patch_tool, + include_web_search_request: config.tools_web_search_request, + use_streamable_shell_tool: config.use_experimental_streamable_shell_tool, + }); let new_turn_context = TurnContext { client, @@ -1172,15 +1173,16 @@ async fn submission_loop( let fresh_turn_context = TurnContext { client, - tools_config: ToolsConfig::new( - &model_family, + tools_config: ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, approval_policy, - sandbox_policy.clone(), - config.include_plan_tool, - config.include_apply_patch_tool, - config.tools_web_search_request, - config.use_experimental_streamable_shell_tool, - ), + sandbox_policy: sandbox_policy.clone(), + include_plan_tool: config.include_plan_tool, + include_apply_patch_tool: config.include_apply_patch_tool, + include_web_search_request: config.tools_web_search_request, + use_streamable_shell_tool: config + .use_experimental_streamable_shell_tool, + }), user_instructions: turn_context.user_instructions.clone(), base_instructions: turn_context.base_instructions.clone(), approval_policy, diff --git a/codex-rs/core/src/openai_tools.rs b/codex-rs/core/src/openai_tools.rs index ca4e947bd2..a9fdb4f0e4 100644 --- a/codex-rs/core/src/openai_tools.rs +++ b/codex-rs/core/src/openai_tools.rs @@ -62,24 +62,35 @@ pub enum ConfigShellToolType { } #[derive(Debug, Clone)] -pub struct ToolsConfig { +pub(crate) struct ToolsConfig { pub shell_type: ConfigShellToolType, pub plan_tool: bool, pub apply_patch_tool_type: Option, pub web_search_request: bool, } +pub(crate) struct ToolsConfigParams<'a> { + pub(crate) model_family: &'a ModelFamily, + pub(crate) approval_policy: AskForApproval, + pub(crate) sandbox_policy: SandboxPolicy, + pub(crate) include_plan_tool: bool, + pub(crate) include_apply_patch_tool: bool, + pub(crate) include_web_search_request: bool, + pub(crate) use_streamable_shell_tool: bool, +} + impl ToolsConfig { - pub fn new( - model_family: &ModelFamily, - approval_policy: AskForApproval, - sandbox_policy: SandboxPolicy, - include_plan_tool: bool, - include_apply_patch_tool: bool, - include_web_search_request: bool, - use_streamable_shell_tool: bool, - ) -> Self { - let mut shell_type = if use_streamable_shell_tool { + pub fn new(params: &ToolsConfigParams) -> Self { + let ToolsConfigParams { + model_family, + approval_policy, + sandbox_policy, + include_plan_tool, + include_apply_patch_tool, + include_web_search_request, + use_streamable_shell_tool, + } = params; + let mut shell_type = if *use_streamable_shell_tool { ConfigShellToolType::StreamableShell } else if model_family.uses_local_shell_tool { ConfigShellToolType::LocalShell @@ -96,7 +107,7 @@ impl ToolsConfig { Some(ApplyPatchToolType::Freeform) => Some(ApplyPatchToolType::Freeform), Some(ApplyPatchToolType::Function) => Some(ApplyPatchToolType::Function), None => { - if include_apply_patch_tool { + if *include_apply_patch_tool { Some(ApplyPatchToolType::Freeform) } else { None @@ -106,9 +117,9 @@ impl ToolsConfig { Self { shell_type, - plan_tool: include_plan_tool, + plan_tool: *include_plan_tool, apply_patch_tool_type, - web_search_request: include_web_search_request, + web_search_request: *include_web_search_request, } } } @@ -585,15 +596,15 @@ mod tests { fn test_get_openai_tools() { let model_family = find_family_for_model("codex-mini-latest") .expect("codex-mini-latest should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - true, - false, - true, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: true, + include_apply_patch_tool: false, + include_web_search_request: true, + use_streamable_shell_tool: false, + }); let tools = get_openai_tools(&config, Some(HashMap::new())); assert_eq_tool_names(&tools, &["local_shell", "update_plan", "web_search"]); @@ -602,15 +613,15 @@ mod tests { #[test] fn test_get_openai_tools_default_shell() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - true, - false, - true, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: true, + include_apply_patch_tool: false, + include_web_search_request: true, + use_streamable_shell_tool: false, + }); let tools = get_openai_tools(&config, Some(HashMap::new())); assert_eq_tool_names(&tools, &["shell", "update_plan", "web_search"]); @@ -619,15 +630,15 @@ mod tests { #[test] fn test_get_openai_tools_mcp_tools() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - false, - false, - true, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: false, + include_apply_patch_tool: false, + include_web_search_request: true, + use_streamable_shell_tool: false, + }); let tools = get_openai_tools( &config, Some(HashMap::from([( @@ -718,14 +729,15 @@ mod tests { #[test] fn test_get_openai_tools_mcp_tools_sorted_by_name() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - false, - false, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: false, + include_apply_patch_tool: false, + include_web_search_request: false, + use_streamable_shell_tool: false, + }); // Intentionally construct a map with keys that would sort alphabetically. let tools_map: HashMap = HashMap::from([ @@ -792,15 +804,15 @@ mod tests { #[test] fn test_mcp_tool_property_missing_type_defaults_to_string() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - false, - false, - true, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: false, + include_apply_patch_tool: false, + include_web_search_request: true, + use_streamable_shell_tool: false, + }); let tools = get_openai_tools( &config, @@ -850,15 +862,15 @@ mod tests { #[test] fn test_mcp_tool_integer_normalized_to_number() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - false, - false, - true, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: false, + include_apply_patch_tool: false, + include_web_search_request: true, + use_streamable_shell_tool: false, + }); let tools = get_openai_tools( &config, @@ -903,15 +915,15 @@ mod tests { #[test] fn test_mcp_tool_array_without_items_gets_default_string_items() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - false, - false, - true, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: false, + include_apply_patch_tool: false, + include_web_search_request: true, + use_streamable_shell_tool: false, + }); let tools = get_openai_tools( &config, @@ -959,15 +971,15 @@ mod tests { #[test] fn test_mcp_tool_anyof_defaults_to_string() { let model_family = find_family_for_model("o3").expect("o3 should be a valid model family"); - let config = ToolsConfig::new( - &model_family, - AskForApproval::Never, - SandboxPolicy::ReadOnly, - false, - false, - true, - /*use_experimental_streamable_shell_tool*/ false, - ); + let config = ToolsConfig::new(&ToolsConfigParams { + model_family: &model_family, + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + include_plan_tool: false, + include_apply_patch_tool: false, + include_web_search_request: true, + use_streamable_shell_tool: false, + }); let tools = get_openai_tools( &config, From aa3d90a193857b6b82c5e79957d19c9b58277b0d Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 24 Aug 2025 22:43:48 -0700 Subject: [PATCH 2/2] fix: Scope ExecSessionManager to Session instead of using global singleton --- codex-rs/core/src/codex.rs | 10 +++++++--- codex-rs/core/src/exec_command/mod.rs | 2 +- codex-rs/core/src/exec_command/session_manager.rs | 3 --- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 9e08ded8eb..49d9c03ca4 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -55,7 +55,7 @@ use crate::exec::StreamOutput; use crate::exec::process_exec_tool_call; use crate::exec_command::EXEC_COMMAND_TOOL_NAME; use crate::exec_command::ExecCommandParams; -use crate::exec_command::SESSION_MANAGER; +use crate::exec_command::ExecSessionManager; use crate::exec_command::WRITE_STDIN_TOOL_NAME; use crate::exec_command::WriteStdinParams; use crate::exec_env::create_env; @@ -269,6 +269,7 @@ pub(crate) struct Session { /// Manager for external MCP servers/tools. mcp_connection_manager: McpConnectionManager, + session_manager: ExecSessionManager, /// External notifier command (will be passed as args to exec()). When /// `None` this feature is disabled. @@ -528,6 +529,7 @@ impl Session { session_id, tx_event: tx_event.clone(), mcp_connection_manager, + session_manager: ExecSessionManager::default(), notify, state: Mutex::new(state), rollout: Mutex::new(rollout_recorder), @@ -2112,7 +2114,8 @@ async fn handle_function_call( }; } }; - let result = SESSION_MANAGER + let result = sess + .session_manager .handle_exec_command_request(exec_params) .await; let function_call_output = crate::exec_command::result_into_payload(result); @@ -2134,7 +2137,8 @@ async fn handle_function_call( }; } }; - let result = SESSION_MANAGER + let result = sess + .session_manager .handle_write_stdin_request(write_stdin_params) .await; let function_call_output: FunctionCallOutputPayload = diff --git a/codex-rs/core/src/exec_command/mod.rs b/codex-rs/core/src/exec_command/mod.rs index 2fd88d4ec5..9cdaa4d31d 100644 --- a/codex-rs/core/src/exec_command/mod.rs +++ b/codex-rs/core/src/exec_command/mod.rs @@ -10,5 +10,5 @@ pub use responses_api::EXEC_COMMAND_TOOL_NAME; pub use responses_api::WRITE_STDIN_TOOL_NAME; pub use responses_api::create_exec_command_tool_for_responses_api; pub use responses_api::create_write_stdin_tool_for_responses_api; -pub use session_manager::SESSION_MANAGER; +pub use session_manager::SessionManager as ExecSessionManager; pub use session_manager::result_into_payload; diff --git a/codex-rs/core/src/exec_command/session_manager.rs b/codex-rs/core/src/exec_command/session_manager.rs index 213b874bfa..5359024bdd 100644 --- a/codex-rs/core/src/exec_command/session_manager.rs +++ b/codex-rs/core/src/exec_command/session_manager.rs @@ -2,7 +2,6 @@ use std::collections::HashMap; use std::io::ErrorKind; use std::io::Read; use std::sync::Arc; -use std::sync::LazyLock; use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicU32; @@ -22,8 +21,6 @@ use crate::exec_command::exec_command_session::ExecCommandSession; use crate::exec_command::session_id::SessionId; use codex_protocol::models::FunctionCallOutputPayload; -pub static SESSION_MANAGER: LazyLock = LazyLock::new(SessionManager::default); - #[derive(Debug, Default)] pub struct SessionManager { next_session_id: AtomicU32,