mirror of
https://github.com/openai/codex.git
synced 2026-02-01 22:47:52 +00:00
chore: drop useless feature flags (#8850)
This commit is contained in:
@@ -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<String>, 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
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
|
||||
@@ -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",
|
||||
|
||||
@@ -47,7 +47,6 @@ pub struct LegacyFeatureToggles {
|
||||
pub experimental_use_freeform_apply_patch: Option<bool>,
|
||||
pub experimental_use_unified_exec_tool: Option<bool>,
|
||||
pub tools_web_search: Option<bool>,
|
||||
pub tools_view_image: Option<bool>,
|
||||
}
|
||||
|
||||
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",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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());
|
||||
|
||||
@@ -22,7 +22,6 @@ pub(crate) struct ToolsConfig {
|
||||
pub apply_patch_tool_type: Option<ApplyPatchToolType>,
|
||||
pub web_search_request: bool,
|
||||
pub web_search_cached: bool,
|
||||
pub include_view_image_tool: bool,
|
||||
pub experimental_supported_tools: Vec<String>,
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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"),
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user