mirror of
https://github.com/openai/codex.git
synced 2026-09-16 12:13:30 +00:00
Preserve selected profile settings over managed new-thread defaults (#44693)
## Why Managed new-thread defaults could overwrite model, reasoning effort, and service tier settings from an explicitly selected profile. ## What changed - Treat effective profile settings as explicit launch choices when applying managed defaults. A profile setting for either `model` or `model_reasoning_effort` opts out of both managed values; `service_tier` remains independent. - Count a profile setting only when it supplies the highest-precedence active value, so settings shadowed by project configuration do not block defaults. ## Testing Add regression coverage for profile precedence at startup, session replacement, and background task creation, plus custom-provider profiles, service tiers, unrelated profile settings, and project settings that shadow profiles. GitOrigin-RevId: 98f036b8b7059bab8598283600dbc6967c71d3e3
This commit is contained in:
@@ -230,6 +230,7 @@ mod managed_worktree_creation;
|
||||
mod misalignment_policy;
|
||||
mod model_defaults;
|
||||
mod new_session;
|
||||
pub(crate) use new_session::has_launch_setting;
|
||||
mod pending_interactive_replay;
|
||||
mod permission_shortcuts;
|
||||
mod pets;
|
||||
|
||||
@@ -5,24 +5,27 @@
|
||||
use super::*;
|
||||
use codex_config::ConfigLayerSource;
|
||||
|
||||
pub(super) fn has_launch_setting(
|
||||
pub(crate) fn has_launch_setting(
|
||||
config: &Config,
|
||||
cli_kv_overrides: &[(String, TomlValue)],
|
||||
key: &str,
|
||||
) -> bool {
|
||||
// A remote server cannot resolve this invocation's explicitly selected local profile.
|
||||
// Only count the profile when it supplies the effective value; project settings can shadow it.
|
||||
cli_kv_overrides.iter().any(|(path, _)| path == key)
|
||||
|| config.config_layer_stack.layers_high_to_low().any(|layer| {
|
||||
layer.disabled_reason.is_none()
|
||||
&& matches!(
|
||||
|| config
|
||||
.config_layer_stack
|
||||
.layers_high_to_low()
|
||||
.find(|layer| layer.config.get(key).is_some())
|
||||
.is_some_and(|layer| {
|
||||
matches!(
|
||||
layer.name,
|
||||
ConfigLayerSource::User {
|
||||
profile: Some(_),
|
||||
..
|
||||
}
|
||||
)
|
||||
&& layer.config.get(key).is_some()
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
pub(super) fn overlay_new_session_defaults(
|
||||
|
||||
@@ -416,6 +416,7 @@ async fn background_task_preserves_explicit_choices_and_managed_defaults() -> Re
|
||||
("saved", "server-model", "high"),
|
||||
("cli_effort", "server-model", "low"),
|
||||
("profile_model", "profile-model", "high"),
|
||||
("profile_managed", "profile-model", "low"),
|
||||
("managed", "managed-model", "medium"),
|
||||
] {
|
||||
let client_home = tempdir()?;
|
||||
@@ -428,7 +429,7 @@ async fn background_task_preserves_explicit_choices_and_managed_defaults() -> Re
|
||||
server_home.path().join("config.toml"),
|
||||
"model = \"server-model\"\nmodel_reasoning_effort = \"high\"\n",
|
||||
)?;
|
||||
if choice == "managed" || choice.starts_with("cli_") {
|
||||
if choice == "managed" || choice == "profile_managed" || choice.starts_with("cli_") {
|
||||
std::fs::write(
|
||||
server_home.path().join("requirements.toml"),
|
||||
"[models.new_thread]\nmodel = \"managed-model\"\nmodel_reasoning_effort = \"medium\"\n",
|
||||
@@ -440,9 +441,16 @@ async fn background_task_preserves_explicit_choices_and_managed_defaults() -> Re
|
||||
"model_reasoning_effort".into(),
|
||||
TomlValue::String("low".into()),
|
||||
)),
|
||||
"profile_model" => {
|
||||
"profile_model" | "profile_managed" => {
|
||||
let path = client_home.path().join("work.config.toml");
|
||||
std::fs::write(&path, "model = \"profile-model\"\n")?;
|
||||
std::fs::write(
|
||||
&path,
|
||||
if choice == "profile_managed" {
|
||||
"model = \"profile-model\"\nmodel_reasoning_effort = \"low\"\n"
|
||||
} else {
|
||||
"model = \"profile-model\"\n"
|
||||
},
|
||||
)?;
|
||||
app.loader_overrides.user_config_path = Some(path.abs());
|
||||
app.loader_overrides.user_config_profile = Some("work".parse()?);
|
||||
}
|
||||
|
||||
@@ -11,8 +11,16 @@ async fn replacement_uses_server_defaults_and_preserves_explicit_launch_settings
|
||||
(Some("medium"), "effort", "server-model", "low"),
|
||||
(None, "profile_model", "profile-model", "high"),
|
||||
(None, "profile_effort", "server-model", "low"),
|
||||
(Some(""), "profile", "managed-model", "low"),
|
||||
(Some("medium"), "profile", "managed-model", "medium"),
|
||||
(Some("medium"), "profile_model", "profile-model", "high"),
|
||||
(Some("medium"), "profile_effort", "server-model", "low"),
|
||||
(Some(""), "profile", "profile-model", "low"),
|
||||
(Some("medium"), "profile", "profile-model", "low"),
|
||||
(
|
||||
Some("medium"),
|
||||
"profile_unrelated",
|
||||
"managed-model",
|
||||
"medium",
|
||||
),
|
||||
] {
|
||||
let (mut app, _events, _ops) = make_test_app_with_channels().await;
|
||||
let server_home = tempdir()?;
|
||||
@@ -68,13 +76,14 @@ async fn replacement_uses_server_defaults_and_preserves_explicit_launch_settings
|
||||
"model_reasoning_effort".to_string(),
|
||||
TomlValue::String("low".to_string()),
|
||||
)),
|
||||
profile @ ("profile" | "profile_model" | "profile_effort") => {
|
||||
profile @ ("profile" | "profile_model" | "profile_effort" | "profile_unrelated") => {
|
||||
let path = client_home.path().join("work.config.toml");
|
||||
std::fs::write(
|
||||
&path,
|
||||
match profile {
|
||||
"profile_model" => "model = \"profile-model\"\n",
|
||||
"profile_effort" => "model_reasoning_effort = \"low\"\n",
|
||||
"profile_unrelated" => "model_verbosity = \"low\"\n",
|
||||
_ => "model = \"profile-model\"\nmodel_reasoning_effort = \"low\"\n",
|
||||
},
|
||||
)?;
|
||||
|
||||
@@ -178,6 +178,8 @@ async fn fresh_startup_uses_server_defaults_with_explicit_and_managed_precedence
|
||||
("cli_effort", true, "server-model", "low"),
|
||||
("profile_model", false, "profile-model", "high"),
|
||||
("profile_effort", false, "server-model", "low"),
|
||||
("profile_model", true, "profile-model", "high"),
|
||||
("profile_effort", true, "server-model", "low"),
|
||||
("managed", true, "managed-model", "medium"),
|
||||
] {
|
||||
let client_home = tempdir()?;
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use crate::app::has_launch_setting;
|
||||
use crate::legacy_core::config::Config;
|
||||
use crate::legacy_core::config::ConfigOverrides;
|
||||
use codex_app_server_protocol::NewThreadModelDefaults;
|
||||
@@ -14,17 +15,16 @@ pub(crate) fn apply_managed_new_thread_defaults(
|
||||
return;
|
||||
};
|
||||
// Managed values are defaults rather than enforcement. Preserve explicit launch choices from
|
||||
// dedicated flags such as `-m` (`harness_overrides`) and generic `-c key=value` settings
|
||||
// (`cli_kv_overrides`), then fill only the fields that were not selected for this invocation.
|
||||
// dedicated flags such as `-m` (`harness_overrides`), generic `-c key=value` settings
|
||||
// (`cli_kv_overrides`), and explicitly selected profiles.
|
||||
// Model and reasoning effort are a compatibility-sensitive pair, so an explicit override of
|
||||
// either opts out of both managed values. For example, `codex -m gpt-5.4` keeps that model and
|
||||
// its existing/default effort, while `-c model_reasoning_effort=low` does not switch to the
|
||||
// managed model. Service tier remains independent and is resolved against the selected model
|
||||
// before the thread starts.
|
||||
let has_cli_override = |key: &str| cli_kv_overrides.iter().any(|(path, _value)| path == key);
|
||||
let has_explicit_model_settings = harness_overrides.model.is_some()
|
||||
|| has_cli_override("model")
|
||||
|| has_cli_override("model_reasoning_effort");
|
||||
|| has_launch_setting(config, cli_kv_overrides, "model")
|
||||
|| has_launch_setting(config, cli_kv_overrides, "model_reasoning_effort");
|
||||
|
||||
if !has_explicit_model_settings && let Some(model) = defaults.model.as_ref() {
|
||||
config.model = Some(model.clone());
|
||||
@@ -35,7 +35,7 @@ pub(crate) fn apply_managed_new_thread_defaults(
|
||||
config.model_reasoning_effort = Some(reasoning_effort.clone());
|
||||
}
|
||||
if harness_overrides.service_tier.is_none()
|
||||
&& !has_cli_override("service_tier")
|
||||
&& !has_launch_setting(config, cli_kv_overrides, "service_tier")
|
||||
&& let Some(service_tier) = defaults.service_tier.as_ref()
|
||||
{
|
||||
config.service_tier = Some(
|
||||
|
||||
@@ -1,6 +1,10 @@
|
||||
use super::*;
|
||||
use crate::legacy_core::config::ConfigBuilder;
|
||||
use codex_config::ConfigLayerSource;
|
||||
use codex_config::LoaderOverrides;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
async fn test_config() -> Config {
|
||||
@@ -108,3 +112,98 @@ async fn explicit_launch_overrides_take_precedence() {
|
||||
|
||||
assert_eq!(actual, expected);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn selected_custom_provider_and_service_tier_preserve_the_profile_model() {
|
||||
let home = tempfile::tempdir().expect("tempdir");
|
||||
std::fs::write(home.path().join("config.toml"), "model = \"base-model\"\n")
|
||||
.expect("base config");
|
||||
std::fs::write(
|
||||
home.path().join("custom.config.toml"),
|
||||
"model = \"custom-model\"\nmodel_provider = \"custom\"\nservice_tier = \"flex\"\n\
|
||||
[model_providers.custom]\nname = \"Custom\"\nbase_url = \"http://127.0.0.1:1/v1\"\nwire_api = \"responses\"\n",
|
||||
)
|
||||
.expect("profile");
|
||||
let mut actual = ConfigBuilder::default()
|
||||
.codex_home(home.path().to_path_buf())
|
||||
.loader_overrides(LoaderOverrides {
|
||||
user_config_path: Some(home.path().join("custom.config.toml").abs()),
|
||||
user_config_profile: Some("custom".parse().expect("profile name")),
|
||||
..LoaderOverrides::without_managed_config_for_tests()
|
||||
})
|
||||
.build()
|
||||
.await
|
||||
.expect("config");
|
||||
let expected = actual.clone();
|
||||
assert_eq!(actual.model.as_deref(), Some("custom-model"));
|
||||
assert_eq!(actual.model_provider_id, "custom");
|
||||
|
||||
apply_managed_new_thread_defaults(
|
||||
&mut actual,
|
||||
Some(&defaults()),
|
||||
&[],
|
||||
&ConfigOverrides::default(),
|
||||
);
|
||||
|
||||
assert_eq!(actual, expected);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn managed_defaults_win_when_a_project_setting_shadows_the_selected_profile() {
|
||||
let home = tempfile::tempdir().expect("tempdir");
|
||||
let project = tempfile::tempdir().expect("project");
|
||||
std::fs::write(
|
||||
home.path().join("work.config.toml"),
|
||||
"model = \"profile-model\"\n",
|
||||
)
|
||||
.expect("profile");
|
||||
std::fs::create_dir(project.path().join(".codex")).expect("project config directory");
|
||||
std::fs::write(
|
||||
project.path().join(".codex/config.toml"),
|
||||
"model = \"project-model\"\n",
|
||||
)
|
||||
.expect("project config");
|
||||
crate::legacy_core::config::set_project_trust_level(
|
||||
home.path(),
|
||||
project.path(),
|
||||
TrustLevel::Trusted,
|
||||
)
|
||||
.expect("trusted project");
|
||||
let mut actual = ConfigBuilder::default()
|
||||
.codex_home(home.path().to_path_buf())
|
||||
.loader_overrides(LoaderOverrides {
|
||||
user_config_path: Some(home.path().join("work.config.toml").abs()),
|
||||
user_config_profile: Some("work".parse().expect("profile name")),
|
||||
..LoaderOverrides::without_managed_config_for_tests()
|
||||
})
|
||||
.harness_overrides(ConfigOverrides {
|
||||
cwd: Some(project.path().to_path_buf()),
|
||||
..ConfigOverrides::default()
|
||||
})
|
||||
.build()
|
||||
.await
|
||||
.expect("config");
|
||||
assert_eq!(actual.model.as_deref(), Some("project-model"));
|
||||
assert!(actual.config_layer_stack.layers_high_to_low().any(|layer| {
|
||||
matches!(
|
||||
layer.name,
|
||||
ConfigLayerSource::User {
|
||||
profile: Some(_),
|
||||
..
|
||||
}
|
||||
) && layer.config.get("model").is_some()
|
||||
}));
|
||||
let mut expected = actual.clone();
|
||||
expected.model = Some("managed-model".to_string());
|
||||
expected.model_reasoning_effort = Some(ReasoningEffort::High);
|
||||
expected.service_tier = Some(ServiceTier::Fast.request_value().to_string());
|
||||
|
||||
apply_managed_new_thread_defaults(
|
||||
&mut actual,
|
||||
Some(&defaults()),
|
||||
&[],
|
||||
&ConfigOverrides::default(),
|
||||
);
|
||||
|
||||
assert_eq!(actual, expected);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user