From bba567cee91c86d7e0f911587a91ded7a705b70a Mon Sep 17 00:00:00 2001 From: jif-oai Date: Fri, 12 Sep 2025 10:38:12 -0700 Subject: [PATCH] bug: fix model save (#3525) Fix those 2 behaviors: 1. The model does not get saved if we don't CTRL + S 2. The reasoning effort get saved --- codex-rs/core/src/codex.rs | 18 ---- codex-rs/core/tests/suite/mod.rs | 1 + codex-rs/core/tests/suite/model_overrides.rs | 92 ++++++++++++++++++++ codex-rs/tui/src/app.rs | 81 ++++++++++++++++- codex-rs/tui/src/chatwidget.rs | 2 +- codex-rs/tui/src/chatwidget/tests.rs | 11 +++ 6 files changed, 185 insertions(+), 20 deletions(-) create mode 100644 codex-rs/core/tests/suite/model_overrides.rs diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index f77e6a40..79bf72ec 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -9,9 +9,6 @@ use std::sync::atomic::AtomicU64; use std::time::Duration; use crate::AuthManager; -use crate::config_edit::CONFIG_KEY_EFFORT; -use crate::config_edit::CONFIG_KEY_MODEL; -use crate::config_edit::persist_non_null_overrides; use crate::event_mapping::map_response_item_to_event_messages; use async_channel::Receiver; use async_channel::Sender; @@ -1170,21 +1167,6 @@ async fn submission_loop( turn_context = Arc::new(new_turn_context); // Optionally persist changes to model / effort - let effort_str = effort.map(|_| effective_effort.to_string()); - - if let Err(e) = persist_non_null_overrides( - &config.codex_home, - config.active_profile.as_deref(), - &[ - (&[CONFIG_KEY_MODEL], model.as_deref()), - (&[CONFIG_KEY_EFFORT], effort_str.as_deref()), - ], - ) - .await - { - warn!("failed to persist overrides: {e:#}"); - } - if cwd.is_some() || approval_policy.is_some() || sandbox_policy.is_some() { sess.record_conversation_items(&[ResponseItem::from(EnvironmentContext::new( cwd, diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index aabf83bb..10661413 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -7,6 +7,7 @@ mod exec; mod exec_stream_events; mod fork_conversation; mod live_cli; +mod model_overrides; mod prompt_caching; mod seatbelt; mod stream_error_allows_next_turn; diff --git a/codex-rs/core/tests/suite/model_overrides.rs b/codex-rs/core/tests/suite/model_overrides.rs new file mode 100644 index 00000000..8bd3a856 --- /dev/null +++ b/codex-rs/core/tests/suite/model_overrides.rs @@ -0,0 +1,92 @@ +use codex_core::CodexAuth; +use codex_core::ConversationManager; +use codex_core::protocol::EventMsg; +use codex_core::protocol::Op; +use codex_core::protocol_config_types::ReasoningEffort; +use core_test_support::load_default_config_for_test; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use tempfile::TempDir; + +const CONFIG_TOML: &str = "config.toml"; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn override_turn_context_does_not_persist_when_config_exists() { + let codex_home = TempDir::new().unwrap(); + let config_path = codex_home.path().join(CONFIG_TOML); + let initial_contents = "model = \"gpt-4o\"\n"; + tokio::fs::write(&config_path, initial_contents) + .await + .expect("seed config.toml"); + + let mut config = load_default_config_for_test(&codex_home); + config.model = "gpt-4o".to_string(); + + let conversation_manager = + ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key")); + let codex = conversation_manager + .new_conversation(config) + .await + .expect("create conversation") + .conversation; + + codex + .submit(Op::OverrideTurnContext { + cwd: None, + approval_policy: None, + sandbox_policy: None, + model: Some("o3".to_string()), + effort: Some(ReasoningEffort::High), + summary: None, + }) + .await + .expect("submit override"); + + codex.submit(Op::Shutdown).await.expect("request shutdown"); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ShutdownComplete)).await; + + let contents = tokio::fs::read_to_string(&config_path) + .await + .expect("read config.toml after override"); + assert_eq!(contents, initial_contents); +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn override_turn_context_does_not_create_config_file() { + let codex_home = TempDir::new().unwrap(); + let config_path = codex_home.path().join(CONFIG_TOML); + assert!( + !config_path.exists(), + "test setup should start without config" + ); + + let config = load_default_config_for_test(&codex_home); + + let conversation_manager = + ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key")); + let codex = conversation_manager + .new_conversation(config) + .await + .expect("create conversation") + .conversation; + + codex + .submit(Op::OverrideTurnContext { + cwd: None, + approval_policy: None, + sandbox_policy: None, + model: Some("o3".to_string()), + effort: Some(ReasoningEffort::Medium), + summary: None, + }) + .await + .expect("submit override"); + + codex.submit(Op::Shutdown).await.expect("request shutdown"); + wait_for_event(&codex, |ev| matches!(ev, EventMsg::ShutdownComplete)).await; + + assert!( + !config_path.exists(), + "override should not create config.toml" + ); +} diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 5680c226..d8819276 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -14,6 +14,7 @@ use codex_core::config::Config; use codex_core::config::persist_model_selection; use codex_core::model_family::find_family_for_model; use codex_core::protocol::TokenUsage; +use codex_core::protocol_config_types::ReasoningEffort as ReasoningEffortConfig; use color_eyre::eyre::Result; use color_eyre::eyre::WrapErr; use crossterm::event::KeyCode; @@ -297,7 +298,7 @@ impl App { self.chat_widget.apply_file_search_result(query, matches); } AppEvent::UpdateReasoningEffort(effort) => { - self.chat_widget.set_reasoning_effort(effort); + self.on_update_reasoning_effort(effort); } AppEvent::UpdateModel(model) => { self.chat_widget.set_model(model.clone()); @@ -336,6 +337,20 @@ impl App { } } + fn on_update_reasoning_effort(&mut self, effort: ReasoningEffortConfig) { + let changed = self.config.model_reasoning_effort != effort; + self.chat_widget.set_reasoning_effort(effort); + self.config.model_reasoning_effort = effort; + if changed { + let show_hint = self.model_saved_to_profile || self.model_saved_to_global; + self.model_saved_to_profile = false; + self.model_saved_to_global = false; + if show_hint { + self.show_model_save_hint(); + } + } + } + async fn persist_model_shortcut(&mut self) { enum SaveScope<'a> { Profile(&'a str), @@ -476,3 +491,67 @@ impl App { }; } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::app_backtrack::BacktrackState; + use crate::chatwidget::tests::make_chatwidget_manual_with_sender; + use crate::file_search::FileSearchManager; + use codex_core::CodexAuth; + use codex_core::ConversationManager; + use ratatui::text::Line; + use std::sync::Arc; + use std::sync::atomic::AtomicBool; + + fn make_test_app() -> App { + let (chat_widget, app_event_tx, _rx, _op_rx) = make_chatwidget_manual_with_sender(); + let config = chat_widget.config_ref().clone(); + + let server = Arc::new(ConversationManager::with_auth(CodexAuth::from_api_key( + "Test API Key", + ))); + let file_search = FileSearchManager::new(config.cwd.clone(), app_event_tx.clone()); + + App { + server, + app_event_tx, + chat_widget, + config, + active_profile: None, + model_saved_to_profile: false, + model_saved_to_global: false, + file_search, + transcript_lines: Vec::>::new(), + overlay: None, + deferred_history_lines: Vec::new(), + has_emitted_history_lines: false, + enhanced_keys_supported: false, + commit_anim_running: Arc::new(AtomicBool::new(false)), + backtrack: BacktrackState::default(), + } + } + + #[test] + fn update_reasoning_effort_updates_config_and_resets_flags() { + let mut app = make_test_app(); + app.model_saved_to_profile = true; + app.model_saved_to_global = true; + app.config.model_reasoning_effort = ReasoningEffortConfig::Medium; + app.chat_widget + .set_reasoning_effort(ReasoningEffortConfig::Medium); + + app.on_update_reasoning_effort(ReasoningEffortConfig::High); + + assert_eq!( + app.config.model_reasoning_effort, + ReasoningEffortConfig::High + ); + assert_eq!( + app.chat_widget.config_ref().model_reasoning_effort, + ReasoningEffortConfig::High + ); + assert!(!app.model_saved_to_profile); + assert!(!app.model_saved_to_global); + } +} diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index f9987a68..1f33facd 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1451,4 +1451,4 @@ fn extract_first_bold(s: &str) -> Option { } #[cfg(test)] -mod tests; +pub(crate) mod tests; diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index dc1da26e..298fc849 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -247,6 +247,17 @@ fn make_chatwidget_manual() -> ( (widget, rx, op_rx) } +pub(crate) fn make_chatwidget_manual_with_sender() -> ( + ChatWidget, + AppEventSender, + tokio::sync::mpsc::UnboundedReceiver, + tokio::sync::mpsc::UnboundedReceiver, +) { + let (widget, rx, op_rx) = make_chatwidget_manual(); + let app_event_tx = widget.app_event_tx.clone(); + (widget, app_event_tx, rx, op_rx) +} + fn drain_insert_history( rx: &mut tokio::sync::mpsc::UnboundedReceiver, ) -> Vec>> {