mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Persist target default reasoning on model upgrade (#19085)
## Why When the TUI upgrade flow moves a user to a newer model, the accepted migration should also persist the target model's default reasoning effort. That keeps the upgraded model and reasoning setting aligned instead of carrying forward a stale previously saved effort from the old model. ## What changed - The accepted model migration path now updates in-memory config, TUI state, and persisted model selection with the target preset's `default_reasoning_effort`. - The upgrade destructuring keeps `reasoning_effort_mapping` explicitly unused because mappings are no longer consulted on accepted migrations. - Added a catalog test that starts with a pre-existing saved reasoning effort and verifies the accepted upgrade overwrites it with the target model default and emits the expected persistence events. - Rebasing onto current `main` also updates a TUI thread-session test helper for the latest `permission_profile` field and `ApprovalsReviewer::AutoReview` rename so CI compiles on the new base. ## Verification - `cargo test -p codex-tui model_catalog` - `cargo test -p codex-tui permission_settings_sync_updates_active_snapshot_without_rewriting_side_thread`
This commit is contained in:
committed by
GitHub
Unverified
parent
2ef2d675d6
commit
993e3f407e
@@ -139,6 +139,28 @@ pub(super) fn target_preset_for_upgrade<'a>(
|
||||
.find(|preset| preset.model == target_model && preset.show_in_picker)
|
||||
}
|
||||
|
||||
pub(super) fn apply_accepted_model_migration(
|
||||
config: &mut Config,
|
||||
app_event_tx: &AppEventSender,
|
||||
from_model: String,
|
||||
target_model: String,
|
||||
target_default_effort: ReasoningEffortConfig,
|
||||
) {
|
||||
app_event_tx.send(AppEvent::PersistModelMigrationPromptAcknowledged {
|
||||
from_model,
|
||||
to_model: target_model.clone(),
|
||||
});
|
||||
|
||||
config.model = Some(target_model.clone());
|
||||
config.model_reasoning_effort = Some(target_default_effort);
|
||||
app_event_tx.send(AppEvent::UpdateModel(target_model.clone()));
|
||||
app_event_tx.send(AppEvent::UpdateReasoningEffort(Some(target_default_effort)));
|
||||
app_event_tx.send(AppEvent::PersistModelSelection {
|
||||
model: target_model,
|
||||
effort: Some(target_default_effort),
|
||||
});
|
||||
}
|
||||
|
||||
pub(super) const MODEL_AVAILABILITY_NUX_MAX_SHOW_COUNT: u32 = 4;
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -218,7 +240,7 @@ pub(super) async fn handle_model_migration_prompt_if_needed(
|
||||
|
||||
if let Some(ModelUpgrade {
|
||||
id: target_model,
|
||||
reasoning_effort_mapping,
|
||||
reasoning_effort_mapping: _,
|
||||
migration_config_key,
|
||||
model_link,
|
||||
upgrade_copy,
|
||||
@@ -263,30 +285,13 @@ pub(super) async fn handle_model_migration_prompt_if_needed(
|
||||
);
|
||||
match run_model_migration_prompt(tui, prompt_copy).await {
|
||||
ModelMigrationOutcome::Accepted => {
|
||||
app_event_tx.send(AppEvent::PersistModelMigrationPromptAcknowledged {
|
||||
from_model: model.to_string(),
|
||||
to_model: target_model.clone(),
|
||||
});
|
||||
|
||||
let mapped_effort = if let Some(reasoning_effort_mapping) = reasoning_effort_mapping
|
||||
&& let Some(reasoning_effort) = config.model_reasoning_effort
|
||||
{
|
||||
reasoning_effort_mapping
|
||||
.get(&reasoning_effort)
|
||||
.cloned()
|
||||
.or(config.model_reasoning_effort)
|
||||
} else {
|
||||
config.model_reasoning_effort
|
||||
};
|
||||
|
||||
config.model = Some(target_model.clone());
|
||||
config.model_reasoning_effort = mapped_effort;
|
||||
app_event_tx.send(AppEvent::UpdateModel(target_model.clone()));
|
||||
app_event_tx.send(AppEvent::UpdateReasoningEffort(mapped_effort));
|
||||
app_event_tx.send(AppEvent::PersistModelSelection {
|
||||
model: target_model.clone(),
|
||||
effort: mapped_effort,
|
||||
});
|
||||
apply_accepted_model_migration(
|
||||
config,
|
||||
app_event_tx,
|
||||
model.to_string(),
|
||||
target_model.clone(),
|
||||
target_preset.default_reasoning_effort,
|
||||
);
|
||||
}
|
||||
ModelMigrationOutcome::Rejected => {
|
||||
app_event_tx.send(AppEvent::PersistModelMigrationPromptAcknowledged {
|
||||
|
||||
@@ -1,7 +1,9 @@
|
||||
use super::*;
|
||||
use assert_matches::assert_matches;
|
||||
use codex_config::types::ModelAvailabilityNuxConfig;
|
||||
use codex_protocol::openai_models::ModelAvailabilityNux;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tokio::sync::mpsc::unbounded_channel;
|
||||
|
||||
fn all_model_presets() -> Vec<ModelPreset> {
|
||||
crate::legacy_core::test_support::all_model_presets().clone()
|
||||
@@ -172,6 +174,61 @@ fn select_model_availability_nux_returns_none_when_all_models_are_exhausted() {
|
||||
assert_eq!(selected, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn accepted_model_migration_persists_target_default_reasoning_effort() {
|
||||
let codex_home = tempdir().expect("temp codex home");
|
||||
let mut config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.build()
|
||||
.await
|
||||
.expect("config");
|
||||
config.model = Some("gpt-5.2".to_string());
|
||||
config.model_reasoning_effort = Some(ReasoningEffortConfig::XHigh);
|
||||
|
||||
let (tx_raw, mut rx) = unbounded_channel();
|
||||
let app_event_tx = AppEventSender::new(tx_raw);
|
||||
|
||||
apply_accepted_model_migration(
|
||||
&mut config,
|
||||
&app_event_tx,
|
||||
"gpt-5.2".to_string(),
|
||||
"gpt-5.4".to_string(),
|
||||
ReasoningEffortConfig::Medium,
|
||||
);
|
||||
|
||||
assert_eq!(config.model.as_deref(), Some("gpt-5.4"));
|
||||
assert_eq!(
|
||||
config.model_reasoning_effort,
|
||||
Some(ReasoningEffortConfig::Medium)
|
||||
);
|
||||
|
||||
let acknowledged = rx.try_recv().expect("acknowledged event");
|
||||
assert_matches!(
|
||||
acknowledged,
|
||||
AppEvent::PersistModelMigrationPromptAcknowledged { from_model, to_model }
|
||||
if from_model == "gpt-5.2" && to_model == "gpt-5.4"
|
||||
);
|
||||
|
||||
let update_model = rx.try_recv().expect("update model event");
|
||||
assert_matches!(
|
||||
update_model,
|
||||
AppEvent::UpdateModel(model) if model == "gpt-5.4"
|
||||
);
|
||||
|
||||
let update_effort = rx.try_recv().expect("update effort event");
|
||||
assert_matches!(
|
||||
update_effort,
|
||||
AppEvent::UpdateReasoningEffort(Some(ReasoningEffortConfig::Medium))
|
||||
);
|
||||
|
||||
let persist_selection = rx.try_recv().expect("persist model selection event");
|
||||
assert_matches!(
|
||||
persist_selection,
|
||||
AppEvent::PersistModelSelection { model, effort }
|
||||
if model == "gpt-5.4" && effort == Some(ReasoningEffortConfig::Medium)
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn model_migration_prompt_respects_hide_flag_and_self_target() {
|
||||
let mut seen = BTreeMap::new();
|
||||
|
||||
@@ -108,6 +108,7 @@ mod tests {
|
||||
approval_policy: AskForApproval::Never,
|
||||
approvals_reviewer: ApprovalsReviewer::User,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
permission_profile: None,
|
||||
cwd: cwd.abs(),
|
||||
instruction_source_paths: Vec::new(),
|
||||
reasoning_effort: None,
|
||||
@@ -155,7 +156,7 @@ mod tests {
|
||||
.insert(side_thread_id, SideThreadState::new(main_thread_id));
|
||||
app.config.permissions.approval_policy =
|
||||
codex_config::Constrained::allow_any(AskForApproval::OnRequest);
|
||||
app.config.approvals_reviewer = ApprovalsReviewer::GuardianSubagent;
|
||||
app.config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
app.config.permissions.sandbox_policy =
|
||||
codex_config::Constrained::allow_any(SandboxPolicy::new_workspace_write_policy());
|
||||
|
||||
@@ -164,7 +165,7 @@ mod tests {
|
||||
|
||||
let expected_main_session = ThreadSessionState {
|
||||
approval_policy: AskForApproval::OnRequest,
|
||||
approvals_reviewer: ApprovalsReviewer::GuardianSubagent,
|
||||
approvals_reviewer: ApprovalsReviewer::AutoReview,
|
||||
sandbox_policy: SandboxPolicy::new_workspace_write_policy(),
|
||||
..main_session
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user