From 1253d196412dbf834c4767c953c3560d870bcc5a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Wed, 7 Jan 2026 19:54:32 +0000 Subject: [PATCH] chore: drop useless feature flags (#8850) --- codex-rs/core/src/codex.rs | 100 ++++++++++----------------- codex-rs/core/src/config/mod.rs | 4 +- codex-rs/core/src/features.rs | 34 --------- codex-rs/core/src/features/legacy.rs | 7 -- codex-rs/core/src/tasks/review.rs | 3 +- codex-rs/core/src/tools/spec.rs | 14 +--- codex-rs/core/tests/suite/client.rs | 2 - codex-rs/core/tests/suite/skills.rs | 43 ++++-------- 8 files changed, 58 insertions(+), 149 deletions(-) diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 98b34d51a2..3251ed118b 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -226,29 +226,22 @@ impl Codex { let (tx_sub, rx_sub) = async_channel::bounded(SUBMISSION_CHANNEL_CAPACITY); let (tx_event, rx_event) = async_channel::unbounded(); - let loaded_skills = if config.features.enabled(Feature::Skills) { - Some(skills_manager.skills_for_config(&config)) - } else { - None - }; + let loaded_skills = skills_manager.skills_for_config(&config); + // let loaded_skills = if config.features.enabled(Feature::Skills) { + // Some(skills_manager.skills_for_config(&config)) + // } else { + // None + // }; - if let Some(outcome) = &loaded_skills { - for err in &outcome.errors { - error!( - "failed to load skill {}: {}", - err.path.display(), - err.message - ); - } + for err in &loaded_skills.errors { + error!( + "failed to load skill {}: {}", + err.path.display(), + err.message + ); } - let user_instructions = get_user_instructions( - &config, - loaded_skills - .as_ref() - .map(|outcome| outcome.skills.as_slice()), - ) - .await; + let user_instructions = get_user_instructions(&config, Some(&loaded_skills.skills)).await; let exec_policy = ExecPolicyManager::load(&config.features, &config.config_layer_stack) .await @@ -1287,10 +1280,6 @@ impl Session { } pub(crate) async fn record_model_warning(&self, message: impl Into, ctx: &TurnContext) { - if !self.enabled(Feature::ModelWarnings) { - return; - } - let item = ResponseItem::Message { id: None, role: "user".to_string(), @@ -1747,7 +1736,7 @@ mod handlers { use crate::codex::spawn_review_thread; use crate::config::Config; - use crate::features::Feature; + use crate::mcp::auth::compute_auth_statuses; use crate::mcp::collect_mcp_snapshot_from_manager; use crate::review_prompts::resolve_review_request; @@ -2037,29 +2026,20 @@ mod handlers { } else { cwds }; - let skills = if sess.enabled(Feature::Skills) { - let skills_manager = &sess.services.skills_manager; - let mut entries = Vec::new(); - for cwd in cwds { - let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; - let errors = super::errors_to_info(&outcome.errors); - let skills = super::skills_to_info(&outcome.skills); - entries.push(SkillsListEntry { - cwd, - skills, - errors, - }); - } - entries - } else { - cwds.into_iter() - .map(|cwd| SkillsListEntry { - cwd, - skills: Vec::new(), - errors: Vec::new(), - }) - .collect() - }; + + let skills_manager = &sess.services.skills_manager; + let mut skills = Vec::new(); + for cwd in cwds { + let outcome = skills_manager.skills_for_cwd(&cwd, force_reload).await; + let errors = super::errors_to_info(&outcome.errors); + let skills_metadata = super::skills_to_info(&outcome.skills); + skills.push(SkillsListEntry { + cwd, + skills: skills_metadata, + errors, + }); + } + let event = Event { id: sub_id, msg: EventMsg::ListSkillsResponse(ListSkillsResponseEvent { skills }), @@ -2212,8 +2192,7 @@ async fn spawn_review_thread( let mut review_features = sess.features.clone(); review_features .disable(crate::features::Feature::WebSearchRequest) - .disable(crate::features::Feature::WebSearchCached) - .disable(crate::features::Feature::ViewImageTool); + .disable(crate::features::Feature::WebSearchCached); let tools_config = ToolsConfig::new(&ToolsConfigParams { model_info: &review_model_info, features: &review_features, @@ -2345,16 +2324,12 @@ pub(crate) async fn run_task( }); sess.send_event(&turn_context, event).await; - let skills_outcome = if sess.enabled(Feature::Skills) { - Some( - sess.services - .skills_manager - .skills_for_cwd(&turn_context.cwd, false) - .await, - ) - } else { - None - }; + let skills_outcome = Some( + sess.services + .skills_manager + .skills_for_cwd(&turn_context.cwd, false) + .await, + ); let SkillInjections { items: skill_items, @@ -2519,7 +2494,7 @@ async fn run_turn( let prompt = Prompt { input, tools: router.specs(), - parallel_tool_calls: model_supports_parallel && sess.enabled(Feature::ParallelToolCalls), + parallel_tool_calls: model_supports_parallel, base_instructions_override: turn_context.base_instructions.clone(), output_schema: turn_context.final_output_json_schema.clone(), }; @@ -3656,8 +3631,7 @@ mod tests { #[tokio::test] async fn record_model_warning_appends_user_message() { let (mut session, turn_context) = make_session_and_context().await; - let mut features = Features::with_defaults(); - features.enable(Feature::ModelWarnings); + let features = Features::with_defaults(); session.features = features; session diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index e2c3bb252d..71b14973d8 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1883,7 +1883,7 @@ trust_level = "trusted" profiles.insert( "work".to_string(), ConfigProfile { - tools_view_image: Some(false), + tools_web_search: Some(false), ..Default::default() }, ); @@ -1899,7 +1899,7 @@ trust_level = "trusted" codex_home.path().to_path_buf(), )?; - assert!(!config.features.enabled(Feature::ViewImageTool)); + assert!(!config.features.enabled(Feature::WebSearchRequest)); Ok(()) } diff --git a/codex-rs/core/src/features.rs b/codex-rs/core/src/features.rs index 2c1dcc9582..7ef74cfb7a 100644 --- a/codex-rs/core/src/features.rs +++ b/codex-rs/core/src/features.rs @@ -60,10 +60,6 @@ pub enum Feature { // Stable. /// Create a ghost commit at each turn. GhostCommit, - /// Include the view_image tool. - ViewImageTool, - /// Send warnings to the model to correct it on the tool usage. - ModelWarnings, /// Enable the default shell tool. ShellTool, @@ -87,14 +83,10 @@ pub enum Feature { RemoteCompaction, /// Refresh remote models and emit AppReady once the list is available. RemoteModels, - /// Allow model to call multiple tools in parallel (only for models supporting it). - ParallelToolCalls, /// Experimental shell snapshotting. ShellSnapshot, /// Experimental TUI v2 (viewport) implementation. Tui2, - /// Enable discovery and injection of skills. - Skills, /// Enforce UTF8 output in Powershell. PowershellUtf8, } @@ -231,7 +223,6 @@ impl Features { experimental_use_freeform_apply_patch: cfg.experimental_use_freeform_apply_patch, experimental_use_unified_exec_tool: cfg.experimental_use_unified_exec_tool, tools_web_search: cfg.tools.as_ref().and_then(|t| t.web_search), - tools_view_image: cfg.tools.as_ref().and_then(|t| t.view_image), ..Default::default() }; base_legacy.apply(&mut features); @@ -247,7 +238,6 @@ impl Features { experimental_use_unified_exec_tool: config_profile.experimental_use_unified_exec_tool, tools_web_search: config_profile.tools_web_search, - tools_view_image: config_profile.tools_view_image, }; profile_legacy.apply(&mut features); if let Some(profile_features) = config_profile.features.as_ref() { @@ -303,30 +293,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: false, }, - FeatureSpec { - id: Feature::ParallelToolCalls, - key: "parallel", - stage: Stage::Stable, - default_enabled: true, - }, - FeatureSpec { - id: Feature::ViewImageTool, - key: "view_image_tool", - stage: Stage::Stable, - default_enabled: true, - }, FeatureSpec { id: Feature::ShellTool, key: "shell_tool", stage: Stage::Stable, default_enabled: true, }, - FeatureSpec { - id: Feature::ModelWarnings, - key: "warnings", - stage: Stage::Stable, - default_enabled: true, - }, FeatureSpec { id: Feature::WebSearchRequest, key: "web_search_request", @@ -396,12 +368,6 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Experimental, default_enabled: false, }, - FeatureSpec { - id: Feature::Skills, - key: "skills", - stage: Stage::Experimental, - default_enabled: true, - }, FeatureSpec { id: Feature::PowershellUtf8, key: "powershell_utf8", diff --git a/codex-rs/core/src/features/legacy.rs b/codex-rs/core/src/features/legacy.rs index 09a982569f..ed508ffb5a 100644 --- a/codex-rs/core/src/features/legacy.rs +++ b/codex-rs/core/src/features/legacy.rs @@ -47,7 +47,6 @@ pub struct LegacyFeatureToggles { pub experimental_use_freeform_apply_patch: Option, pub experimental_use_unified_exec_tool: Option, pub tools_web_search: Option, - pub tools_view_image: Option, } impl LegacyFeatureToggles { @@ -76,12 +75,6 @@ impl LegacyFeatureToggles { self.tools_web_search, "tools.web_search", ); - set_if_some( - features, - Feature::ViewImageTool, - self.tools_view_image, - "tools.view_image", - ); } } diff --git a/codex-rs/core/src/tasks/review.rs b/codex-rs/core/src/tasks/review.rs index 4a2b587af4..c68fe1ce96 100644 --- a/codex-rs/core/src/tasks/review.rs +++ b/codex-rs/core/src/tasks/review.rs @@ -85,8 +85,7 @@ async fn start_review_conversation( // re-enable blocked tools (web search, view image). sub_agent_config .features - .disable(crate::features::Feature::WebSearchRequest) - .disable(crate::features::Feature::ViewImageTool); + .disable(crate::features::Feature::WebSearchRequest); // Set explicit review rubric for the sub-agent sub_agent_config.base_instructions = Some(crate::REVIEW_PROMPT.to_string()); diff --git a/codex-rs/core/src/tools/spec.rs b/codex-rs/core/src/tools/spec.rs index a92a624a7e..846025d58e 100644 --- a/codex-rs/core/src/tools/spec.rs +++ b/codex-rs/core/src/tools/spec.rs @@ -22,7 +22,6 @@ pub(crate) struct ToolsConfig { pub apply_patch_tool_type: Option, pub web_search_request: bool, pub web_search_cached: bool, - pub include_view_image_tool: bool, pub experimental_supported_tools: Vec, } @@ -40,7 +39,6 @@ impl ToolsConfig { let include_apply_patch_tool = features.enabled(Feature::ApplyPatchFreeform); let include_web_search_request = features.enabled(Feature::WebSearchRequest); let include_web_search_cached = features.enabled(Feature::WebSearchCached); - let include_view_image_tool = features.enabled(Feature::ViewImageTool); let shell_type = if !features.enabled(Feature::ShellTool) { ConfigShellToolType::Disabled @@ -72,7 +70,6 @@ impl ToolsConfig { apply_patch_tool_type, web_search_request: include_web_search_request, web_search_cached: include_web_search_cached, - include_view_image_tool, experimental_supported_tools: model_info.experimental_supported_tools.clone(), } } @@ -1107,10 +1104,8 @@ pub(crate) fn build_specs( }); } - if config.include_view_image_tool { - builder.push_spec_with_parallel_support(create_view_image_tool(), true); - builder.register_handler("view_image", view_image_handler); - } + builder.push_spec_with_parallel_support(create_view_image_tool(), true); + builder.register_handler("view_image", view_image_handler); if let Some(mcp_tools) = mcp_tools { let mut entries: Vec<(String, mcp_types::Tool)> = mcp_tools.into_iter().collect(); @@ -1236,7 +1231,6 @@ mod tests { let mut features = Features::with_defaults(); features.enable(Feature::UnifiedExec); features.enable(Feature::WebSearchRequest); - features.enable(Feature::ViewImageTool); let config = ToolsConfig::new(&ToolsConfigParams { model_info: &model_info, features: &features, @@ -1556,7 +1550,6 @@ mod tests { let config = test_config(); let model_info = ModelsManager::construct_model_info_offline("gpt-5-codex", &config); let mut features = Features::with_defaults(); - features.disable(Feature::ViewImageTool); features.enable(Feature::UnifiedExec); let tools_config = ToolsConfig::new(&ToolsConfigParams { model_info: &model_info, @@ -1575,8 +1568,7 @@ mod tests { fn test_test_model_info_includes_sync_tool() { let config = test_config(); let model_info = ModelsManager::construct_model_info_offline("test-gpt-5-codex", &config); - let mut features = Features::with_defaults(); - features.disable(Feature::ViewImageTool); + let features = Features::with_defaults(); let tools_config = ToolsConfig::new(&ToolsConfigParams { model_info: &model_info, features: &features, diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 0e5fe0d9bd..2c08083ba5 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -15,7 +15,6 @@ use codex_core::WireApi; use codex_core::auth::AuthCredentialsStoreMode; use codex_core::built_in_model_providers; use codex_core::error::CodexErr; -use codex_core::features::Feature; use codex_core::models_manager::manager::ModelsManager; use codex_core::protocol::EventMsg; use codex_core::protocol::Op; @@ -683,7 +682,6 @@ async fn skills_append_to_instructions() { let mut config = load_default_config_for_test(&codex_home).await; config.model_provider = model_provider; config.cwd = codex_home.path().to_path_buf(); - config.features.enable(Feature::Skills); let thread_manager = ThreadManager::with_models_provider_and_home( CodexAuth::from_api_key("Test API Key"), diff --git a/codex-rs/core/tests/suite/skills.rs b/codex-rs/core/tests/suite/skills.rs index e64b5db3e7..8e9266ee86 100644 --- a/codex-rs/core/tests/suite/skills.rs +++ b/codex-rs/core/tests/suite/skills.rs @@ -2,7 +2,6 @@ #![allow(clippy::unwrap_used, clippy::expect_used)] use anyhow::Result; -use codex_core::features::Feature; use codex_core::protocol::AskForApproval; use codex_core::protocol::Op; use codex_core::protocol::SandboxPolicy; @@ -41,13 +40,9 @@ async fn user_turn_includes_skill_instructions() -> Result<()> { let server = start_mock_server().await; let skill_body = "skill body"; - let mut builder = test_codex() - .with_config(|config| { - config.features.enable(Feature::Skills); - }) - .with_pre_build_hook(|home| { - write_skill(home, "demo", "demo skill", skill_body); - }); + let mut builder = test_codex().with_pre_build_hook(|home| { + write_skill(home, "demo", "demo skill", skill_body); + }); let test = builder.build(&server).await?; let skill_path = test.codex_home_path().join("skills/demo/SKILL.md"); @@ -111,15 +106,11 @@ async fn skill_load_errors_surface_in_session_configured() -> Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; - let mut builder = test_codex() - .with_config(|config| { - config.features.enable(Feature::Skills); - }) - .with_pre_build_hook(|home| { - let skill_dir = home.join("skills").join("broken"); - fs::create_dir_all(&skill_dir).unwrap(); - fs::write(skill_dir.join("SKILL.md"), "not yaml").unwrap(); - }); + let mut builder = test_codex().with_pre_build_hook(|home| { + let skill_dir = home.join("skills").join("broken"); + fs::create_dir_all(&skill_dir).unwrap(); + fs::write(skill_dir.join("SKILL.md"), "not yaml").unwrap(); + }); let test = builder.build(&server).await?; test.codex @@ -169,17 +160,13 @@ async fn list_skills_includes_system_cache_entries() -> Result<()> { const SYSTEM_SKILL_NAME: &str = "skill-creator"; let server = start_mock_server().await; - let mut builder = test_codex() - .with_config(|config| { - config.features.enable(Feature::Skills); - }) - .with_pre_build_hook(|home| { - let system_skill_path = system_skill_md_path(home, SYSTEM_SKILL_NAME); - assert!( - !system_skill_path.exists(), - "expected embedded system skills not yet installed, but {system_skill_path:?} exists" - ); - }); + let mut builder = test_codex().with_pre_build_hook(|home| { + let system_skill_path = system_skill_md_path(home, SYSTEM_SKILL_NAME); + assert!( + !system_skill_path.exists(), + "expected embedded system skills not yet installed, but {system_skill_path:?} exists" + ); + }); let test = builder.build(&server).await?; let system_skill_path = system_skill_md_path(test.codex_home_path(), SYSTEM_SKILL_NAME);