From 7b20db942a33cb17925d76305d515d986a7b02c8 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Sun, 24 Aug 2025 22:43:42 -0700 Subject: [PATCH] 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 e175f550..9e08ded8 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 ca4e947b..a9fdb4f0 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,