mirror of
https://github.com/openai/codex.git
synced 2026-11-03 19:53:25 +00:00
Address review comments
This commit is contained in:
@@ -72,6 +72,7 @@ use codex_core::config::edit::ConfigEditsBuilder;
|
||||
use codex_core::config::find_codex_home;
|
||||
use codex_core::config::resolve_profile_v2_config_path;
|
||||
use codex_features::FEATURES;
|
||||
use codex_features::Feature;
|
||||
use codex_features::Stage;
|
||||
use codex_features::is_known_feature_key;
|
||||
use codex_login::AuthManager;
|
||||
@@ -868,15 +869,25 @@ impl FeatureToggles {
|
||||
let mut v = Vec::new();
|
||||
for feature in &self.enable {
|
||||
Self::validate_feature(feature)?;
|
||||
v.push(format!("features.{feature}=true"));
|
||||
let key = Self::config_override_key(feature);
|
||||
v.push(format!("{key}=true"));
|
||||
}
|
||||
for feature in &self.disable {
|
||||
Self::validate_feature(feature)?;
|
||||
v.push(format!("features.{feature}=false"));
|
||||
let key = Self::config_override_key(feature);
|
||||
v.push(format!("{key}=false"));
|
||||
}
|
||||
Ok(v)
|
||||
}
|
||||
|
||||
fn config_override_key(feature: &str) -> String {
|
||||
if feature == Feature::SystemProxy.key() {
|
||||
format!("features.{feature}.enabled")
|
||||
} else {
|
||||
format!("features.{feature}")
|
||||
}
|
||||
}
|
||||
|
||||
fn validate_feature(feature: &str) -> anyhow::Result<()> {
|
||||
if is_known_feature_key(feature) {
|
||||
Ok(())
|
||||
@@ -3826,6 +3837,22 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn feature_toggles_system_proxy_generate_structured_overrides() {
|
||||
let toggles = FeatureToggles {
|
||||
enable: vec!["system_proxy".to_string()],
|
||||
disable: vec!["system_proxy".to_string()],
|
||||
};
|
||||
let overrides = toggles.to_overrides().expect("valid features");
|
||||
assert_eq!(
|
||||
overrides,
|
||||
vec![
|
||||
"features.system_proxy.enabled=true".to_string(),
|
||||
"features.system_proxy.enabled=false".to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn feature_toggles_accept_legacy_linux_sandbox_flag() {
|
||||
let toggles = FeatureToggles {
|
||||
|
||||
@@ -73,6 +73,45 @@ async fn features_disable_writes_feature_flag_to_config() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn system_proxy_feature_toggles_preserve_mode() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let config_path = codex_home.path().join("config.toml");
|
||||
std::fs::write(
|
||||
&config_path,
|
||||
r#"[features.system_proxy]
|
||||
enabled = false
|
||||
mode = "system"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let mut cmd = codex_command(codex_home.path())?;
|
||||
cmd.args(["features", "enable", "system_proxy"])
|
||||
.assert()
|
||||
.success();
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(&config_path)?,
|
||||
r#"[features.system_proxy]
|
||||
enabled = true
|
||||
mode = "system"
|
||||
"#
|
||||
);
|
||||
|
||||
let mut cmd = codex_command(codex_home.path())?;
|
||||
cmd.args(["features", "disable", "system_proxy"])
|
||||
.assert()
|
||||
.success();
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(config_path)?,
|
||||
r#"[features.system_proxy]
|
||||
enabled = false
|
||||
mode = "system"
|
||||
"#
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn features_enable_under_development_feature_prints_warning() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -1473,6 +1473,91 @@ mode = "system"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn bootstrap_system_proxy_honors_feature_requirements() -> std::io::Result<()> {
|
||||
let configured = ConfigToml {
|
||||
features: Some(
|
||||
toml::from_str(
|
||||
r#"
|
||||
[system_proxy]
|
||||
enabled = true
|
||||
mode = "system"
|
||||
"#,
|
||||
)
|
||||
.expect("valid features"),
|
||||
),
|
||||
..Default::default()
|
||||
};
|
||||
let disabled = Sourced::new(
|
||||
FeatureRequirementsToml {
|
||||
entries: BTreeMap::from([("system_proxy".to_string(), false)]),
|
||||
},
|
||||
RequirementSource::Unknown,
|
||||
);
|
||||
assert_eq!(
|
||||
resolve_bootstrap_system_proxy_config(&configured, Some(&disabled))?,
|
||||
None
|
||||
);
|
||||
|
||||
let configured = ConfigToml {
|
||||
features: Some(
|
||||
toml::from_str(
|
||||
r#"
|
||||
[system_proxy]
|
||||
mode = "system"
|
||||
"#,
|
||||
)
|
||||
.expect("valid features"),
|
||||
),
|
||||
..Default::default()
|
||||
};
|
||||
let enabled = Sourced::new(
|
||||
FeatureRequirementsToml {
|
||||
entries: BTreeMap::from([("system_proxy".to_string(), true)]),
|
||||
},
|
||||
RequirementSource::Unknown,
|
||||
);
|
||||
assert_eq!(
|
||||
resolve_bootstrap_system_proxy_config(&configured, Some(&enabled))?,
|
||||
Some(SystemProxyFeatureConfigToml {
|
||||
enabled: Some(true),
|
||||
mode: Some(SystemProxyFeatureModeToml::System),
|
||||
})
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn system_proxy_cli_enable_preserves_configured_mode() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"
|
||||
[features.system_proxy]
|
||||
enabled = false
|
||||
mode = "system"
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let config = ConfigBuilder::without_managed_config_for_tests()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cli_overrides(vec![(
|
||||
"features.system_proxy.enabled".to_string(),
|
||||
toml::Value::Boolean(true),
|
||||
)])
|
||||
.build()
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
config.system_proxy,
|
||||
Some(SystemProxyFeatureConfigToml {
|
||||
enabled: Some(true),
|
||||
mode: Some(SystemProxyFeatureModeToml::System),
|
||||
})
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn experimental_network_requirements_enable_proxy_without_feature() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -6,6 +6,7 @@ use codex_config::types::McpServerConfig;
|
||||
use codex_config::types::SessionPickerViewMode;
|
||||
use codex_config::types::ToolSuggestDisabledTool;
|
||||
use codex_features::FEATURES;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::config_types::Personality;
|
||||
use codex_protocol::config_types::ServiceTier;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
@@ -882,6 +883,18 @@ impl ConfigEditsBuilder {
|
||||
/// persisting `false`, so the config does not pin the feature once it
|
||||
/// graduates to globally enabled.
|
||||
pub fn set_feature_enabled(mut self, key: &str, enabled: bool) -> Self {
|
||||
if key == Feature::SystemProxy.key() {
|
||||
self.edits.push(ConfigEdit::SetPath {
|
||||
segments: vec![
|
||||
"features".to_string(),
|
||||
key.to_string(),
|
||||
"enabled".to_string(),
|
||||
],
|
||||
value: value(enabled),
|
||||
});
|
||||
return self;
|
||||
}
|
||||
|
||||
let segments = vec!["features".to_string(), key.to_string()];
|
||||
let is_default_false_feature = FEATURES
|
||||
.iter()
|
||||
|
||||
@@ -2456,7 +2456,8 @@ fn resolved_system_proxy_config(
|
||||
/// cloud config is available.
|
||||
pub fn resolve_bootstrap_system_proxy_config(
|
||||
cfg: &ConfigToml,
|
||||
) -> Option<SystemProxyFeatureConfigToml> {
|
||||
feature_requirements: Option<&Sourced<FeatureRequirementsToml>>,
|
||||
) -> std::io::Result<Option<SystemProxyFeatureConfigToml>> {
|
||||
let configured_features = Features::from_sources(
|
||||
FeatureConfigSource {
|
||||
features: cfg.features.as_ref(),
|
||||
@@ -2465,7 +2466,9 @@ pub fn resolve_bootstrap_system_proxy_config(
|
||||
FeatureConfigSource::default(),
|
||||
FeatureOverrides::default(),
|
||||
);
|
||||
resolved_system_proxy_config_from_features(cfg, &configured_features)
|
||||
let features =
|
||||
ManagedFeatures::from_configured(configured_features, feature_requirements.cloned())?;
|
||||
Ok(resolved_system_proxy_config(cfg, &features))
|
||||
}
|
||||
|
||||
pub(crate) fn resolve_web_search_mode_for_turn(
|
||||
|
||||
Reference in New Issue
Block a user