mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Honor server model defaults when starting fresh TUI threads (#50013)
## Why Fresh TUI threads should use the app server's model settings unless the launch explicitly overrides them. Sending resolved configuration values in `thread/start` turns defaults into explicit overrides. ## What changed - Track explicit launch choices for `model`, `model_provider`, and `model_reasoning_effort`, and omit unselected settings from the initial `thread/start` request. - Preserve accepted model migrations as explicit model and reasoning-effort choices. - Keep explicit profile launches on the existing resolved-configuration path. ## Testing Update startup request tests to cover omitted defaults, CLI model and provider overrides, repeated model overrides, reasoning-effort overrides, accepted migrations, and explicit profiles. GitOrigin-RevId: 4e2661ba42322a80e2b769a55c62f61a576762b0
This commit is contained in:
@@ -18,6 +18,7 @@ fn spawn_startup_thread_start(
|
||||
app_server: &AppServerSession,
|
||||
local_settings: crate::local_settings::LocalSettings,
|
||||
config: Config,
|
||||
launch_choices: crate::app_server_session::StartupLaunchChoices,
|
||||
app_event_tx: AppEventSender,
|
||||
worktree: Option<crate::ManagedTuiWorktree>,
|
||||
) {
|
||||
@@ -25,7 +26,6 @@ fn spawn_startup_thread_start(
|
||||
let thread_params_mode = app_server.thread_params_mode();
|
||||
let remote_cwd_override = app_server.remote_cwd_override().map(Path::to_path_buf);
|
||||
let thread_tool_transport = app_server.thread_tool_transport();
|
||||
let model_provider_override = app_server.model_provider_override.clone();
|
||||
tokio::spawn(async move {
|
||||
let result = crate::app_server_session::start_thread_with_request_handle(
|
||||
request_handle,
|
||||
@@ -34,7 +34,7 @@ fn spawn_startup_thread_start(
|
||||
thread_params_mode,
|
||||
remote_cwd_override,
|
||||
thread_tool_transport,
|
||||
model_provider_override,
|
||||
launch_choices,
|
||||
)
|
||||
.await
|
||||
.and_then(|started| {
|
||||
@@ -331,6 +331,11 @@ impl App {
|
||||
&harness_overrides,
|
||||
);
|
||||
}
|
||||
let mut launch_choices = crate::app_server_session::StartupLaunchChoices::from_launch(
|
||||
&cli_kv_overrides,
|
||||
&harness_overrides,
|
||||
&loader_overrides,
|
||||
);
|
||||
let mut model = startup_model(&config, &bootstrap, startup_defaults.server_defaults_read);
|
||||
let available_models = bootstrap.available_models;
|
||||
let remote_connection = crate::status::remote_connection::remote_connection_status_value(
|
||||
@@ -361,6 +366,7 @@ impl App {
|
||||
tui,
|
||||
&mut config,
|
||||
&local_settings,
|
||||
&mut launch_choices,
|
||||
model.as_str(),
|
||||
&app_event_tx,
|
||||
&available_models,
|
||||
@@ -461,6 +467,7 @@ impl App {
|
||||
&app_server,
|
||||
local_settings.clone(),
|
||||
config.clone(),
|
||||
launch_choices,
|
||||
app_event_tx.clone(),
|
||||
managed_worktree.clone(),
|
||||
);
|
||||
|
||||
@@ -217,6 +217,7 @@ pub(super) fn target_preset_for_upgrade<'a>(
|
||||
|
||||
pub(super) fn apply_accepted_model_migration(
|
||||
config: &mut Config,
|
||||
launch_choices: &mut crate::app_server_session::StartupLaunchChoices,
|
||||
app_event_tx: &AppEventSender,
|
||||
from_model: String,
|
||||
target_model: String,
|
||||
@@ -227,6 +228,8 @@ pub(super) fn apply_accepted_model_migration(
|
||||
to_model: target_model.clone(),
|
||||
});
|
||||
|
||||
launch_choices.model = Some(target_model.clone());
|
||||
launch_choices.effort = Some(serde_json::json!(target_default_effort));
|
||||
config.model = Some(target_model.clone());
|
||||
config.model_reasoning_effort = Some(target_default_effort.clone());
|
||||
app_event_tx.send(AppEvent::UpdateModel(target_model.clone()));
|
||||
@@ -310,6 +313,7 @@ pub(super) async fn handle_model_migration_prompt_if_needed(
|
||||
tui: &mut tui::Tui,
|
||||
config: &mut Config,
|
||||
local_settings: &crate::local_settings::LocalSettings,
|
||||
launch_choices: &mut crate::app_server_session::StartupLaunchChoices,
|
||||
model: &str,
|
||||
app_event_tx: &AppEventSender,
|
||||
available_models: &[ModelPreset],
|
||||
@@ -367,6 +371,7 @@ pub(super) async fn handle_model_migration_prompt_if_needed(
|
||||
ModelMigrationOutcome::Accepted => {
|
||||
apply_accepted_model_migration(
|
||||
config,
|
||||
launch_choices,
|
||||
app_event_tx,
|
||||
model.to_string(),
|
||||
target_model.clone(),
|
||||
|
||||
@@ -367,7 +367,7 @@ async fn lost_initial_thread_reply_keeps_startup_draft_offline() -> Result<()> {
|
||||
ThreadParamsMode::Remote,
|
||||
/*remote_cwd_override*/ None,
|
||||
session.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
crate::app_server_session::StartupLaunchChoices::default(),
|
||||
)
|
||||
.await;
|
||||
let mut tui = crate::tui::test_support::make_test_tui()?;
|
||||
|
||||
@@ -439,6 +439,7 @@ async fn accepted_model_migration_persists_target_default_reasoning_effort() ->
|
||||
let app_event_tx = AppEventSender::new(tx_raw);
|
||||
apply_accepted_model_migration(
|
||||
&mut config,
|
||||
&mut crate::app_server_session::StartupLaunchChoices::default(),
|
||||
&app_event_tx,
|
||||
current_model,
|
||||
upgrade.id,
|
||||
|
||||
@@ -64,7 +64,7 @@ async fn daybreak_command_persists_and_confirms_each_selection() -> Result<()> {
|
||||
crate::app_server_session::ThreadParamsMode::Embedded,
|
||||
/*remote_cwd_override*/ None,
|
||||
server.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
crate::app_server_session::StartupLaunchChoices::default(),
|
||||
)
|
||||
.await?;
|
||||
assert!(startup.session.daybreak_enabled);
|
||||
@@ -1024,7 +1024,7 @@ async fn external_transport_registers_dynamic_tools_and_finds_task_mentions() ->
|
||||
crate::app_server_session::ThreadParamsMode::Embedded,
|
||||
/*remote_cwd_override*/ None,
|
||||
app_server.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
crate::app_server_session::StartupLaunchChoices::default(),
|
||||
)
|
||||
.await?;
|
||||
assert!(startup.task_tools_available);
|
||||
@@ -1305,7 +1305,7 @@ async fn local_daemon_registers_approval_gated_mcp_tools_for_both_start_paths()
|
||||
crate::app_server_session::ThreadParamsMode::Embedded,
|
||||
/*remote_cwd_override*/ None,
|
||||
app_server.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
crate::app_server_session::StartupLaunchChoices::default(),
|
||||
)
|
||||
.await?;
|
||||
assert!(startup.task_tools_available);
|
||||
@@ -1696,7 +1696,7 @@ async fn older_external_server_starts_without_unsupported_dynamic_tools_or_histo
|
||||
crate::app_server_session::ThreadParamsMode::Embedded,
|
||||
/*remote_cwd_override*/ None,
|
||||
app_server.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
crate::app_server_session::StartupLaunchChoices::default(),
|
||||
)
|
||||
.await?;
|
||||
assert!(!startup.task_tools_available);
|
||||
|
||||
@@ -175,21 +175,50 @@ async fn run_until_thread_start(
|
||||
|
||||
#[tokio::test]
|
||||
async fn fresh_startup_uses_server_defaults_with_explicit_and_managed_precedence() -> Result<()> {
|
||||
for (choice, managed, expected_model, expected_effort) in [
|
||||
("saved", false, "server-model", "high"),
|
||||
("cli_model", true, "cli-model", "high"),
|
||||
("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"),
|
||||
for (choice, managed, expected_model, expected_effort, expected_provider) in [
|
||||
("saved", false, None, None, None),
|
||||
("cli_model", true, Some("cli-model"), None, None),
|
||||
("kv_model", false, Some("kv-model"), None, None),
|
||||
(
|
||||
"migration",
|
||||
false,
|
||||
Some("upgraded-model"),
|
||||
Some("high"),
|
||||
None,
|
||||
),
|
||||
("cli_effort", true, None, Some("low"), None),
|
||||
("cli_provider", false, None, None, Some("openai")),
|
||||
("kv_provider", false, None, None, Some("openai")),
|
||||
(
|
||||
"profile_model",
|
||||
false,
|
||||
Some("profile-model"),
|
||||
Some("high"),
|
||||
None,
|
||||
),
|
||||
(
|
||||
"profile_effort",
|
||||
false,
|
||||
Some("server-model"),
|
||||
Some("low"),
|
||||
None,
|
||||
),
|
||||
("managed", true, None, None, None),
|
||||
] {
|
||||
let client_home = tempdir()?;
|
||||
let server_home = tempdir()?;
|
||||
std::fs::write(
|
||||
client_home.path().join("config.toml"),
|
||||
"model = \"client-model\"\nmodel_reasoning_effort = \"low\"\n",
|
||||
r#"
|
||||
model = "client-model"
|
||||
model_reasoning_effort = "low"
|
||||
model_provider = "client-provider"
|
||||
[model_providers.client-provider]
|
||||
name = "Client provider"
|
||||
base_url = "http://127.0.0.1:9/v1"
|
||||
wire_api = "responses"
|
||||
requires_openai_auth = false
|
||||
"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
server_home.path().join("config.toml"),
|
||||
@@ -206,6 +235,13 @@ async fn fresh_startup_uses_server_defaults_with_explicit_and_managed_precedence
|
||||
let mut loader_overrides = LoaderOverrides::without_managed_config_for_tests();
|
||||
match choice {
|
||||
"cli_model" => harness_overrides.model = Some("cli-model".to_string()),
|
||||
"kv_model" => cli_kv_overrides.extend([
|
||||
("model".into(), "earlier-model".into()),
|
||||
("model".into(), "kv-model".into()),
|
||||
]),
|
||||
"migration" => harness_overrides.model = Some("old-model".into()),
|
||||
"cli_provider" => harness_overrides.model_provider = Some("openai".into()),
|
||||
"kv_provider" => cli_kv_overrides.push(("model_provider".into(), "openai".into())),
|
||||
"cli_effort" => cli_kv_overrides.push((
|
||||
"model_reasoning_effort".to_string(),
|
||||
TomlValue::String("low".to_string()),
|
||||
@@ -225,6 +261,11 @@ async fn fresh_startup_uses_server_defaults_with_explicit_and_managed_precedence
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
let mut launch_choices = crate::app_server_session::StartupLaunchChoices::from_launch(
|
||||
&cli_kv_overrides,
|
||||
&harness_overrides,
|
||||
&loader_overrides,
|
||||
);
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(client_home.path().to_path_buf())
|
||||
.loader_overrides(loader_overrides)
|
||||
@@ -249,7 +290,7 @@ async fn fresh_startup_uses_server_defaults_with_explicit_and_managed_precedence
|
||||
)
|
||||
.await?;
|
||||
server = server.with_remote_cwd_override(Some(server_config.cwd.to_path_buf()));
|
||||
let bootstrap = server.bootstrap(&config).await?;
|
||||
server.bootstrap(&config).await?;
|
||||
assert!(
|
||||
prepare_fresh_startup_config(
|
||||
&mut config,
|
||||
@@ -261,32 +302,42 @@ async fn fresh_startup_uses_server_defaults_with_explicit_and_managed_precedence
|
||||
.await?
|
||||
.server_defaults_read
|
||||
);
|
||||
let selected_model = startup_model(&config, &bootstrap, /*server_defaults_read*/ true);
|
||||
let started = crate::app_server_session::start_thread_with_request_handle(
|
||||
if choice == "migration" {
|
||||
let (tx, _rx) = tokio::sync::mpsc::unbounded_channel();
|
||||
apply_accepted_model_migration(
|
||||
&mut config,
|
||||
&mut launch_choices,
|
||||
&AppEventSender::new(tx),
|
||||
"old-model".into(),
|
||||
"upgraded-model".into(),
|
||||
ReasoningEffortConfig::High,
|
||||
);
|
||||
}
|
||||
crate::app_server_session::start_thread_with_request_handle(
|
||||
server.request_handle(),
|
||||
&crate::local_settings::LocalSettings::from(&config),
|
||||
config,
|
||||
server.thread_params_mode(),
|
||||
server.remote_cwd_override().map(Path::to_path_buf),
|
||||
server.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
launch_choices,
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(selected_model, expected_model, "{choice}");
|
||||
let starts = recorded_params(&requests, "thread/start");
|
||||
assert_eq!(starts.len(), 1, "{choice}");
|
||||
assert_eq!(
|
||||
(
|
||||
&starts[0]["model"],
|
||||
&starts[0]["config"]["model_reasoning_effort"]
|
||||
starts[0]["model"].clone(),
|
||||
starts[0]["config"].get("model_reasoning_effort").cloned(),
|
||||
starts[0]["modelProvider"].clone()
|
||||
),
|
||||
(
|
||||
&serde_json::json!(expected_model),
|
||||
&serde_json::json!(expected_effort)
|
||||
serde_json::json!(expected_model),
|
||||
expected_effort.map(serde_json::Value::from),
|
||||
serde_json::json!(expected_provider),
|
||||
),
|
||||
"{choice}"
|
||||
);
|
||||
assert_eq!(started.session.model, expected_model, "{choice}");
|
||||
assert_eq!(
|
||||
recorded_params(&requests, "config/read")
|
||||
.into_iter()
|
||||
@@ -372,7 +423,7 @@ async fn fresh_startup_reads_destination_and_cleared_model_uses_catalog() -> Res
|
||||
server.thread_params_mode(),
|
||||
server.remote_cwd_override().map(Path::to_path_buf),
|
||||
server.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
crate::app_server_session::StartupLaunchChoices::default(),
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(started.session.model, selected_model);
|
||||
@@ -389,7 +440,7 @@ async fn fresh_startup_reads_destination_and_cleared_model_uses_catalog() -> Res
|
||||
let starts = recorded_params(&requests, "thread/start");
|
||||
assert_eq!(starts.len(), 1);
|
||||
assert_eq!(starts[0]["model"], serde_json::Value::Null);
|
||||
assert_eq!(starts[0]["config"]["model_reasoning_effort"], "high");
|
||||
assert!(starts[0]["config"].get("model_reasoning_effort").is_none());
|
||||
let (mut app, _, _) = make_test_app_with_channels().await;
|
||||
app.chat_widget.handle_thread_session_quiet(started.session);
|
||||
if !remote {
|
||||
@@ -457,10 +508,7 @@ async fn fresh_startup_falls_back_only_for_unsupported_config_read() -> Result<(
|
||||
&starts[0]["model"],
|
||||
&starts[0]["config"]["model_reasoning_effort"]
|
||||
),
|
||||
(
|
||||
&serde_json::json!("client-model"),
|
||||
&serde_json::json!("low")
|
||||
)
|
||||
(&serde_json::Value::Null, &serde_json::Value::Null)
|
||||
);
|
||||
tokio::time::timeout(Duration::from_secs(/*secs*/ 15), proxy).await???;
|
||||
}
|
||||
@@ -520,10 +568,7 @@ async fn startup_reads_server_defaults_before_starting_thread() -> Result<()> {
|
||||
&starts[0]["model"],
|
||||
&starts[0]["config"]["model_reasoning_effort"]
|
||||
),
|
||||
(
|
||||
&serde_json::json!("server-model"),
|
||||
&serde_json::json!("high")
|
||||
),
|
||||
(&serde_json::Value::Null, &serde_json::Value::Null),
|
||||
);
|
||||
tokio::time::timeout(Duration::from_secs(/*secs*/ 15), proxy).await???;
|
||||
Ok(())
|
||||
|
||||
@@ -13,6 +13,7 @@ pub(crate) mod provider_selection;
|
||||
mod provider_selection_tests;
|
||||
mod realtime;
|
||||
mod rollout_history;
|
||||
mod startup_launch;
|
||||
mod thread_list;
|
||||
mod web_search;
|
||||
|
||||
@@ -25,6 +26,7 @@ pub(crate) use history::HISTORY_ITEM_SCAN_LIMIT;
|
||||
pub(crate) use history::HistoryHydrationScope;
|
||||
pub(crate) use history::INITIAL_HISTORY_TURN_LIMIT;
|
||||
pub(crate) use history::thread_items_page_params;
|
||||
pub(crate) use startup_launch::StartupLaunchChoices;
|
||||
|
||||
use crate::app_event::PermissionProfileSelection;
|
||||
use crate::app_event_sender::AppEventSender;
|
||||
@@ -1729,7 +1731,7 @@ pub(crate) async fn start_thread_with_request_handle(
|
||||
thread_params_mode: ThreadParamsMode,
|
||||
remote_cwd_override: Option<PathBuf>,
|
||||
thread_tool_transport: ThreadToolTransport,
|
||||
model_provider_override: Option<String>,
|
||||
launch_choices: StartupLaunchChoices,
|
||||
) -> Result<AppServerStartedThread> {
|
||||
let request_id = RequestId::String(format!("startup-thread-start-{}", Uuid::new_v4()));
|
||||
let mut params = thread_start_params_from_config(
|
||||
@@ -1738,7 +1740,7 @@ pub(crate) async fn start_thread_with_request_handle(
|
||||
remote_cwd_override.as_deref(),
|
||||
/*session_start_source*/ None,
|
||||
);
|
||||
params.model_provider = model_provider_override.or(params.model_provider);
|
||||
launch_choices.configure(&mut params);
|
||||
params.daybreak_enabled = (config.daybreak_enabled && !config.ephemeral).then_some(true);
|
||||
thread_tool_transport.configure(&mut params);
|
||||
let (response, _history_support, task_tools_available) =
|
||||
|
||||
@@ -70,7 +70,7 @@ async fn lifecycle_metadata_uses_local_prompt_history() -> Result<()> {
|
||||
mode,
|
||||
/*remote_cwd_override*/ None,
|
||||
app_server.thread_tool_transport(),
|
||||
/*model_provider_override*/ None,
|
||||
crate::app_server_session::StartupLaunchChoices::default(),
|
||||
)
|
||||
.await?;
|
||||
assert_eq!(startup.session.message_history, expected);
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
//! First-thread model overrides come from invocation inputs, independently of local config.
|
||||
//! Explicit profile launches retain their existing resolved-config path until server profile
|
||||
//! selection is available for connected launches.
|
||||
|
||||
use crate::legacy_core::config::ConfigOverrides;
|
||||
use codex_app_server_protocol::ThreadStartParams;
|
||||
use codex_config::LoaderOverrides;
|
||||
|
||||
#[derive(Default)]
|
||||
pub(crate) struct StartupLaunchChoices {
|
||||
pub(crate) model: Option<String>,
|
||||
pub(crate) effort: Option<serde_json::Value>,
|
||||
provider: Option<String>,
|
||||
selected_profile: bool,
|
||||
}
|
||||
|
||||
impl StartupLaunchChoices {
|
||||
pub(crate) fn from_launch(
|
||||
cli_overrides: &[(String, toml::Value)],
|
||||
harness_overrides: &ConfigOverrides,
|
||||
loader_overrides: &LoaderOverrides,
|
||||
) -> Self {
|
||||
if loader_overrides.user_config_profile.is_some() {
|
||||
return Self {
|
||||
provider: harness_overrides.model_provider.clone(),
|
||||
selected_profile: true,
|
||||
..Self::default()
|
||||
};
|
||||
}
|
||||
let value = |key| {
|
||||
cli_overrides
|
||||
.iter()
|
||||
.rev()
|
||||
.find(|(path, _)| path == key)
|
||||
.map(|(_, value)| value)
|
||||
};
|
||||
Self {
|
||||
model: harness_overrides.model.clone().or_else(|| {
|
||||
value("model")
|
||||
.and_then(toml::Value::as_str)
|
||||
.map(str::to_owned)
|
||||
}),
|
||||
effort: value("model_reasoning_effort")
|
||||
.and_then(|value| serde_json::to_value(value).ok()),
|
||||
provider: harness_overrides.model_provider.clone().or_else(|| {
|
||||
value("model_provider")
|
||||
.and_then(toml::Value::as_str)
|
||||
.map(str::to_owned)
|
||||
}),
|
||||
selected_profile: false,
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn configure(self, params: &mut ThreadStartParams) {
|
||||
if self.selected_profile {
|
||||
params.model_provider = self.provider.or(params.model_provider.take());
|
||||
return;
|
||||
}
|
||||
params.model = self.model;
|
||||
params.model_provider = self.provider;
|
||||
if let Some(overrides) = params.config.as_mut() {
|
||||
overrides.remove("model_reasoning_effort");
|
||||
}
|
||||
if let Some(effort) = self.effort {
|
||||
params
|
||||
.config
|
||||
.get_or_insert_default()
|
||||
.insert("model_reasoning_effort".into(), effort);
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user