From b995d06050ee3db5c6298f4a007975da94399c71 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Fri, 4 Sep 2026 04:34:46 +0000 Subject: [PATCH] Preserve TUI sessions while starting replacement threads (#42671) ## Why Starting a replacement thread can fail while reading configuration or calling `thread/start`. The active thread and any composed input should remain available when that happens. Remote app servers also need new threads to use their own effective defaults instead of stale client-side model settings. ## What changed - Read effective server configuration before starting a replacement thread, while preserving explicit model and reasoning-effort launch settings. - Keep the current thread, configuration, subscriptions, and composer input intact until `thread/start` succeeds. - Fall back to local defaults when an older server does not support `config/read`. ## Testing Added coverage for server and managed defaults, explicit launch overrides, remote working directories, older servers, and failures during configuration reads or thread startup. GitOrigin-RevId: 507a918ea2bf79c9f0adbb189bc611ecf57b757a --- codex-rs/tui/src/app.rs | 1 + codex-rs/tui/src/app/new_session.rs | 80 ++++++ codex-rs/tui/src/app/session_lifecycle.rs | 40 ++- .../tui/src/app/tests/new_session_tests.rs | 248 ++++++++++++++++++ .../app/tests/session_lifecycle_requests.rs | 43 ++- 5 files changed, 396 insertions(+), 16 deletions(-) create mode 100644 codex-rs/tui/src/app/new_session.rs create mode 100644 codex-rs/tui/src/app/tests/new_session_tests.rs diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 493d281f9b..f777550c2b 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -226,6 +226,7 @@ mod input; mod loaded_threads; mod misalignment_policy; mod model_defaults; +mod new_session; mod pending_interactive_replay; mod permission_shortcuts; mod pets; diff --git a/codex-rs/tui/src/app/new_session.rs b/codex-rs/tui/src/app/new_session.rs new file mode 100644 index 0000000000..2dc66fba02 --- /dev/null +++ b/codex-rs/tui/src/app/new_session.rs @@ -0,0 +1,80 @@ +//! New-session configuration from server defaults and explicit launch settings. +//! +//! Build the replacement configuration without replacing the active task's settings. + +use super::*; +use codex_config::ConfigLayerSource; + +impl App { + pub(super) async fn load_new_session_config( + &mut self, + app_server: &AppServerSession, + ) -> Result { + let cwd = self.chat_widget.config_ref().cwd.to_path_buf(); + let defaults_cwd = match app_server.thread_params_mode() { + crate::app_server_session::ThreadParamsMode::Embedded => cwd.as_path(), + crate::app_server_session::ThreadParamsMode::Remote => { + app_server.remote_cwd_override().unwrap_or(Path::new(".")) + } + }; + // config/read resolves relative paths on the server. With no remote launch override, + // "." uses the same server process directory as thread/start's omitted cwd. + let defaults = match crate::config_update::read_effective_config( + app_server.request_handle(), + defaults_cwd.display().to_string(), + ) + .await + { + Ok(response) => Some(response.config), + Err(err) + if matches!( + err.downcast_ref::(), + Some(TypedRequestError::Server { source, .. }) + if source.code == -32601 + || source.code == -32600 + && source.message.contains("config/read") + && (source.message.contains("unknown variant") + || source.message.contains("unknown method")) + ) => + { + // Older servers can still start threads using the existing local defaults. + None + } + Err(err) => return Err(err), + }; + // Stage local preferences and permission carryover without changing the active task. + let mut config = match self.rebuild_config_for_cwd(cwd).await { + Ok(config) => config, + Err(err) => { + tracing::warn!(%err, "failed to refresh local settings before a new thread"); + self.config.clone() + } + }; + self.apply_runtime_policy_overrides(&mut config, RuntimePolicyOverrideScope::All); + config.service_tier = self.chat_widget.configured_service_tier(); + if let Some(defaults) = defaults { + // A remote server cannot resolve this invocation's explicitly selected local profile. + let has_launch_setting = |key: &str| { + self.cli_kv_overrides.iter().any(|(path, _)| path == key) + || config.config_layer_stack.layers_high_to_low().any(|layer| { + layer.disabled_reason.is_none() + && matches!( + layer.name, + ConfigLayerSource::User { + profile: Some(_), + .. + } + ) + && layer.config.get(key).is_some() + }) + }; + if self.harness_overrides.model.is_none() && !has_launch_setting("model") { + config.model = defaults.model; + } + if !has_launch_setting("model_reasoning_effort") { + config.model_reasoning_effort = defaults.model_reasoning_effort; + } + } + Ok(config) + } +} diff --git a/codex-rs/tui/src/app/session_lifecycle.rs b/codex-rs/tui/src/app/session_lifecycle.rs index 606e2edfd9..1516e3f11d 100644 --- a/codex-rs/tui/src/app/session_lifecycle.rs +++ b/codex-rs/tui/src/app/session_lifecycle.rs @@ -831,10 +831,18 @@ impl App { // Start a fresh in-memory session while preserving resumability via persisted rollout // history. If an initial message is provided, `enqueue_primary_thread_session` suppresses it // until the new session is configured and any replayed turns have been rendered. - self.refresh_in_memory_config_from_disk_best_effort("starting a new thread") - .await; - let model = self.chat_widget.current_model().to_string(); - let mut config = self.fresh_session_config(); + let mut config = match self.load_new_session_config(app_server).await { + Ok(config) => config, + Err(err) => { + if let Some(message) = initial_user_message { + self.chat_widget.restore_user_message_to_composer(message); + } + self.chat_widget + .add_error_message(format!("Failed to read new session defaults: {err}")); + tui.frame_requester().schedule_frame(); + return; + } + }; apply_managed_new_thread_defaults( &mut config, app_server.managed_new_thread_defaults(), @@ -847,15 +855,6 @@ impl App { self.chat_widget.thread_name(), self.chat_widget.rollout_path().as_deref(), ); - self.shutdown_current_thread(app_server).await; - let tracked_thread_ids: Vec = - self.thread_event_channels.keys().copied().collect(); - for thread_id in tracked_thread_ids { - if let Err(err) = app_server.thread_unsubscribe(thread_id).await { - tracing::warn!("failed to unsubscribe tracked thread {thread_id}: {err}"); - } - } - self.config = config.clone(); match app_server .start_thread_with_session_start_source( &config, @@ -865,6 +864,17 @@ impl App { .await { Ok(mut started) => { + self.shutdown_current_thread(app_server).await; + let tracked_thread_ids: Vec = + self.thread_event_channels.keys().copied().collect(); + for thread_id in tracked_thread_ids { + if let Err(err) = app_server.thread_unsubscribe(thread_id).await { + tracing::warn!("failed to unsubscribe tracked thread {thread_id}: {err}"); + } + } + self.local_settings = crate::local_settings::LocalSettings::from(&config); + self.config = config; + let name_error = if let Some(name) = new_thread_name { match app_server .thread_set_name(started.session.thread_id, name.clone()) @@ -913,7 +923,9 @@ impl App { self.chat_widget.add_error_message(format!( "Failed to start a fresh session through the app server: {err}" )); - self.config.model = Some(model); + if let Some(message) = initial_user_message { + self.chat_widget.restore_user_message_to_composer(message); + } } } tui.frame_requester().schedule_frame(); diff --git a/codex-rs/tui/src/app/tests/new_session_tests.rs b/codex-rs/tui/src/app/tests/new_session_tests.rs new file mode 100644 index 0000000000..cd2707392a --- /dev/null +++ b/codex-rs/tui/src/app/tests/new_session_tests.rs @@ -0,0 +1,248 @@ +//! Replacement starts use server defaults and preserve the current task on failure. + +use super::*; +use pretty_assertions::assert_eq; + +#[tokio::test] +async fn replacement_uses_server_defaults_and_preserves_explicit_launch_settings() -> Result<()> { + for (managed_effort, explicit, expected_model, expected_effort) in [ + (None, "saved", "server-model", "high"), + (Some("medium"), "model", "cli-model", "high"), + (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"), + ] { + let (mut app, _events, _ops) = make_test_app_with_channels().await; + let server_home = tempdir()?; + let client_home = tempdir()?; + std::fs::write( + server_home.path().join("work.config.toml"), + "model = \"server-model\"\nmodel_reasoning_effort = \"high\"\n", + )?; + std::fs::write( + client_home.path().join("config.toml"), + "model = \"stale-client-model\"\nmodel_reasoning_effort = \"low\"\n", + )?; + if let Some(effort) = managed_effort { + let effort = if effort.is_empty() { + String::new() + } else { + format!("model_reasoning_effort = {effort:?}\n") + }; + std::fs::write( + server_home.path().join("requirements.toml"), + format!("[models.new_thread]\nmodel = \"managed-model\"\n{effort}"), + )?; + } + let mut server_config = app.config.clone(); + server_config.codex_home = server_home.path().to_path_buf().abs(); + server_config.sqlite = SqliteConfig::new_for_testing(server_home.path().abs()); + let (mut server, requests, proxy) = start_recording_app_server_with_history( + &server_config, + HistoryCapabilities::Current, + /*blocked_thread_list*/ None, + /*failed_thread_name*/ None, + crate::app_server_session::ThreadParamsMode::Remote, + LoaderOverrides { + user_config_path: Some(server_home.path().join("work.config.toml").abs()), + user_config_profile: Some("work".parse()?), + system_requirements_path: Some(server_home.path().join("requirements.toml")), + ..LoaderOverrides::default() + }, + ) + .await?; + server = server.with_remote_cwd_override(Some(server_config.cwd.to_path_buf())); + server.bootstrap(&server_config).await?; + app.config.codex_home = client_home.path().to_path_buf().abs(); + app.config.sqlite = SqliteConfig::new_for_testing(client_home.path().abs()); + app.chat_widget + .handle_thread_session_quiet(test_thread_session( + ThreadId::new(), + client_home.path().to_path_buf(), + )); + match explicit { + "model" => app.harness_overrides.model = Some("cli-model".to_string()), + "effort" => app.cli_kv_overrides.push(( + "model_reasoning_effort".to_string(), + TomlValue::String("low".to_string()), + )), + profile @ ("profile" | "profile_model" | "profile_effort") => { + 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", + _ => "model = \"profile-model\"\nmodel_reasoning_effort = \"low\"\n", + }, + )?; + app.loader_overrides.user_config_path = Some(path.abs()); + app.loader_overrides.user_config_profile = Some("work".parse()?); + } + _ => {} + } + let mut tui = crate::tui::test_support::make_test_tui()?; + app.start_fresh_session_with_summary_hint( + &mut tui, + &mut server, + /*session_start_source*/ None, + /*initial_user_message*/ None, + /*new_thread_name*/ None, + ) + .await; + let starts = recorded_params(&requests, "thread/start"); + assert_eq!(starts.len(), 1); + assert_eq!( + ( + &starts[0]["model"], + &starts[0]["config"]["model_reasoning_effort"] + ), + ( + &serde_json::json!(expected_model), + &serde_json::json!(expected_effort) + ), + ); + assert_eq!( + recorded_params(&requests, "config/read"), + vec![serde_json::json!({"cwd": server_config.cwd.display().to_string()})], + ); + if explicit == "saved" { + let rendered = render_bottom_popup(&app.chat_widget, /*width*/ 80) + .replace(&server_config.cwd.display().to_string(), ""); + insta::assert_snapshot!(rendered, @r" + › Ask Codex to do anything + + server-model high · + "); + } + server.shutdown().await?; + proxy.await??; + } + Ok(()) +} + +#[tokio::test] +async fn replacement_failure_keeps_current_task_and_restores_input() -> Result<()> { + for fail_read in [false, true] { + let (mut app, mut events, _ops) = make_test_app_with_channels().await; + let home = tempdir()?; + app.config.codex_home = home.path().to_path_buf().abs(); + app.config.sqlite = SqliteConfig::new_for_testing(home.path().abs()); + let current = ThreadId::new(); + app.enqueue_primary_thread_session( + test_thread_session(current, app.config.cwd.to_path_buf()), + Vec::new(), + ) + .await?; + while events.try_recv().is_ok() {} + let original_config = app.config.clone(); + let (mut server, requests, proxy) = start_recording_app_server_with_history( + &app.config, + HistoryCapabilities::ThreadStartFails, + /*blocked_thread_list*/ None, + /*failed_thread_name*/ None, + crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), + ) + .await?; + if fail_read { + std::fs::write(home.path().join("config.toml"), "invalid = [")?; + } + let mut tui = crate::tui::test_support::make_test_tui()?; + app.start_fresh_session_with_summary_hint( + &mut tui, + &mut server, + /*session_start_source*/ None, + Some("keep this request".into()), + /*new_thread_name*/ None, + ) + .await; + assert_eq!(app.chat_widget.thread_id(), Some(current)); + assert_eq!(app.active_thread_id, Some(current)); + assert!(app.active_thread_rx.is_some()); + assert_eq!(app.config, original_config); + assert_eq!( + app.chat_widget.composer_text_with_pending(), + "keep this request" + ); + assert!(recorded_params(&requests, "thread/unsubscribe").is_empty()); + assert_eq!( + recorded_params(&requests, "thread/start").len(), + usize::from(!fail_read) + ); + let errors = std::iter::from_fn(|| events.try_recv().ok()) + .filter_map(|event| match event { + AppEvent::InsertHistoryCell(cell) => { + Some(lines_to_single_string(&cell.display_lines(/*width*/ 1000))) + } + _ => None, + }) + .collect::>() + .join("\n"); + if fail_read { + insta::assert_snapshot!(errors, @"■ Failed to read new session defaults: config/read failed in TUI"); + } else { + insta::assert_snapshot!(errors, @"■ Failed to start a fresh session through the app server: thread/start failed during TUI bootstrap: thread/start failed: replacement unavailable (code -32603)"); + } + server.shutdown().await?; + proxy.await??; + } + Ok(()) +} + +#[tokio::test] +async fn replacement_preserves_remote_launch_paths_and_older_servers() -> Result<()> { + for (remote_cwd, capabilities) in [ + (None, HistoryCapabilities::Current), + (Some(PathBuf::from(".")), HistoryCapabilities::Current), + (None, HistoryCapabilities::ConfigReadUnsupported(-32601)), + (None, HistoryCapabilities::ConfigReadUnsupported(-32600)), + ] { + let (mut app, _events, _ops) = make_test_app_with_channels().await; + let home = tempdir()?; + app.config.codex_home = home.path().to_path_buf().abs(); + app.config.sqlite = SqliteConfig::new_for_testing(home.path().abs()); + std::fs::write( + home.path().join("config.toml"), + "model = \"configured-model\"\n", + )?; + let (server, requests, proxy) = start_recording_app_server_with_history( + &app.config, + capabilities, + /*blocked_thread_list*/ None, + /*failed_thread_name*/ None, + crate::app_server_session::ThreadParamsMode::Remote, + LoaderOverrides::default(), + ) + .await?; + let mut server = server.with_remote_cwd_override(remote_cwd.clone()); + let mut tui = crate::tui::test_support::make_test_tui()?; + app.start_fresh_session_with_summary_hint( + &mut tui, + &mut server, + /*session_start_source*/ None, + /*initial_user_message*/ None, + /*new_thread_name*/ None, + ) + .await; + assert_eq!( + recorded_params(&requests, "config/read"), + vec![serde_json::json!({"cwd": "."}),] + ); + let starts = recorded_params(&requests, "thread/start"); + assert_eq!(starts.len(), 1); + assert_eq!( + (&starts[0]["cwd"], &starts[0]["model"]), + ( + &serde_json::json!(remote_cwd), + &serde_json::json!("configured-model") + ), + ); + assert_eq!(app.chat_widget.current_model(), "configured-model"); + server.shutdown().await?; + proxy.await??; + } + Ok(()) +} diff --git a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs index a92304eceb..3a751d426e 100644 --- a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs +++ b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs @@ -49,6 +49,8 @@ enum HistoryCapabilities { LegacyDynamicToolsAndHistory, ForkHydrationFails, ThreadListFails, + ThreadStartFails, + ConfigReadUnsupported(i64), } /// Returns and resets `(thread/loaded/list, thread/read)` request counts. @@ -78,6 +80,7 @@ pub(super) async fn start_recording_app_server( blocked_thread_list, failed_thread_name, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await } @@ -91,6 +94,7 @@ pub(super) async fn start_recording_remote_app_server( /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Remote, + LoaderOverrides::default(), ) .await } @@ -102,6 +106,7 @@ async fn start_recording_app_server_with_history( mut blocked_thread_list: Option<(ThreadId, oneshot::Sender<()>, oneshot::Receiver<()>)>, failed_thread_name: Option<&'static str>, thread_params_mode: crate::app_server_session::ThreadParamsMode, + loader_overrides: LoaderOverrides, ) -> Result { let state_db = crate::init_state_db_for_app_server_target(config, &crate::AppServerTarget::Embedded) @@ -110,7 +115,7 @@ async fn start_recording_app_server_with_history( codex_arg0::Arg0DispatchPaths::default(), config.clone(), Vec::new(), - codex_config::LoaderOverrides::default(), + loader_overrides, /*strict_config*/ false, codex_config::CloudConfigBundleLoader::default(), codex_feedback::CodexFeedback::new(), @@ -185,7 +190,30 @@ async fn start_recording_app_server_with_history( .is_some_and(|tools| { tools.iter().any(|tool| tool["type"] == "namespace") }); - let response = if request.method == "thread/list" + let response = if let HistoryCapabilities::ConfigReadUnsupported(code) = + history_capabilities + && request.method == "config/read" + { + JSONRPCMessage::Error(JSONRPCError { + id: request_id, + error: JSONRPCErrorError { + code, + data: None, + message: "unknown variant `config/read`".to_string(), + }, + }) + } else if history_capabilities == HistoryCapabilities::ThreadStartFails + && request.method == "thread/start" + { + JSONRPCMessage::Error(JSONRPCError { + id: request_id, + error: JSONRPCErrorError { + code: -32603, + data: None, + message: "replacement unavailable".to_string(), + }, + }) + } else if request.method == "thread/list" && std::mem::take(&mut reject_thread_list) { JSONRPCMessage::Error(JSONRPCError { @@ -1013,6 +1041,7 @@ async fn older_external_server_starts_without_unsupported_dynamic_tools_or_histo /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; @@ -1863,6 +1892,7 @@ async fn remote_legacy_history_start_negotiates_once_for_resume_and_fork() -> Re /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; @@ -1955,6 +1985,7 @@ async fn remote_legacy_history_start_retries_unsupported_paginated_variant() -> /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; @@ -1986,6 +2017,7 @@ async fn assert_remote_legacy_history_retry(request: LegacyHistoryRequest) -> Re /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; @@ -2053,6 +2085,7 @@ async fn paginated_fork_survives_post_response_hydration_failure() -> Result<()> /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; @@ -2391,6 +2424,7 @@ async fn agents_overview_stop_uses_history_mode_for_turn_lookup() -> Result<()> /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; @@ -2437,6 +2471,7 @@ async fn agents_overview_seeds_loaded_threads_when_recent_listing_is_unavailable /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; let started = app_server.start_thread(&app.config).await?; @@ -2514,6 +2549,7 @@ async fn agents_overview_stop_uses_full_history_after_legacy_negotiation() -> Re /*blocked_thread_list*/ None, /*failed_thread_name*/ None, crate::app_server_session::ThreadParamsMode::Embedded, + LoaderOverrides::default(), ) .await?; app_server.start_thread(&app.config).await?; @@ -3392,3 +3428,6 @@ fn session_lifecycle_avoids_redundant_subagent_metadata_reads() -> Result<()> { .join() .expect("session lifecycle request test thread") } + +#[path = "new_session_tests.rs"] +mod new_session_tests;