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
This commit is contained in:
@@ -9,9 +9,6 @@ use std::sync::atomic::AtomicU64;
|
|||||||
use std::time::Duration;
|
use std::time::Duration;
|
||||||
|
|
||||||
use crate::AuthManager;
|
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 crate::event_mapping::map_response_item_to_event_messages;
|
||||||
use async_channel::Receiver;
|
use async_channel::Receiver;
|
||||||
use async_channel::Sender;
|
use async_channel::Sender;
|
||||||
@@ -1170,21 +1167,6 @@ async fn submission_loop(
|
|||||||
turn_context = Arc::new(new_turn_context);
|
turn_context = Arc::new(new_turn_context);
|
||||||
|
|
||||||
// Optionally persist changes to model / effort
|
// 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() {
|
if cwd.is_some() || approval_policy.is_some() || sandbox_policy.is_some() {
|
||||||
sess.record_conversation_items(&[ResponseItem::from(EnvironmentContext::new(
|
sess.record_conversation_items(&[ResponseItem::from(EnvironmentContext::new(
|
||||||
cwd,
|
cwd,
|
||||||
|
|||||||
@@ -7,6 +7,7 @@ mod exec;
|
|||||||
mod exec_stream_events;
|
mod exec_stream_events;
|
||||||
mod fork_conversation;
|
mod fork_conversation;
|
||||||
mod live_cli;
|
mod live_cli;
|
||||||
|
mod model_overrides;
|
||||||
mod prompt_caching;
|
mod prompt_caching;
|
||||||
mod seatbelt;
|
mod seatbelt;
|
||||||
mod stream_error_allows_next_turn;
|
mod stream_error_allows_next_turn;
|
||||||
|
|||||||
92
codex-rs/core/tests/suite/model_overrides.rs
Normal file
92
codex-rs/core/tests/suite/model_overrides.rs
Normal file
@@ -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"
|
||||||
|
);
|
||||||
|
}
|
||||||
@@ -14,6 +14,7 @@ use codex_core::config::Config;
|
|||||||
use codex_core::config::persist_model_selection;
|
use codex_core::config::persist_model_selection;
|
||||||
use codex_core::model_family::find_family_for_model;
|
use codex_core::model_family::find_family_for_model;
|
||||||
use codex_core::protocol::TokenUsage;
|
use codex_core::protocol::TokenUsage;
|
||||||
|
use codex_core::protocol_config_types::ReasoningEffort as ReasoningEffortConfig;
|
||||||
use color_eyre::eyre::Result;
|
use color_eyre::eyre::Result;
|
||||||
use color_eyre::eyre::WrapErr;
|
use color_eyre::eyre::WrapErr;
|
||||||
use crossterm::event::KeyCode;
|
use crossterm::event::KeyCode;
|
||||||
@@ -297,7 +298,7 @@ impl App {
|
|||||||
self.chat_widget.apply_file_search_result(query, matches);
|
self.chat_widget.apply_file_search_result(query, matches);
|
||||||
}
|
}
|
||||||
AppEvent::UpdateReasoningEffort(effort) => {
|
AppEvent::UpdateReasoningEffort(effort) => {
|
||||||
self.chat_widget.set_reasoning_effort(effort);
|
self.on_update_reasoning_effort(effort);
|
||||||
}
|
}
|
||||||
AppEvent::UpdateModel(model) => {
|
AppEvent::UpdateModel(model) => {
|
||||||
self.chat_widget.set_model(model.clone());
|
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) {
|
async fn persist_model_shortcut(&mut self) {
|
||||||
enum SaveScope<'a> {
|
enum SaveScope<'a> {
|
||||||
Profile(&'a str),
|
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::<Line<'static>>::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);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -1451,4 +1451,4 @@ fn extract_first_bold(s: &str) -> Option<String> {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[cfg(test)]
|
#[cfg(test)]
|
||||||
mod tests;
|
pub(crate) mod tests;
|
||||||
|
|||||||
@@ -247,6 +247,17 @@ fn make_chatwidget_manual() -> (
|
|||||||
(widget, rx, op_rx)
|
(widget, rx, op_rx)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
pub(crate) fn make_chatwidget_manual_with_sender() -> (
|
||||||
|
ChatWidget,
|
||||||
|
AppEventSender,
|
||||||
|
tokio::sync::mpsc::UnboundedReceiver<AppEvent>,
|
||||||
|
tokio::sync::mpsc::UnboundedReceiver<Op>,
|
||||||
|
) {
|
||||||
|
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(
|
fn drain_insert_history(
|
||||||
rx: &mut tokio::sync::mpsc::UnboundedReceiver<AppEvent>,
|
rx: &mut tokio::sync::mpsc::UnboundedReceiver<AppEvent>,
|
||||||
) -> Vec<Vec<ratatui::text::Line<'static>>> {
|
) -> Vec<Vec<ratatui::text::Line<'static>>> {
|
||||||
|
|||||||
Reference in New Issue
Block a user