From 26ef31a9020ccec301ca2066701d047880aa954d Mon Sep 17 00:00:00 2001 From: Charles Cunningham Date: Tue, 27 Jan 2026 13:21:26 -0800 Subject: [PATCH] Require remote personality metadata --- codex-rs/core/src/models_manager/manager.rs | 53 ++++----------- .../core/src/models_manager/model_info.rs | 21 ------ codex-rs/core/tests/suite/personality.rs | 42 ++++++------ codex-rs/tui/src/chatwidget/tests.rs | 67 ++++++++++++++++++- 4 files changed, 100 insertions(+), 83 deletions(-) diff --git a/codex-rs/core/src/models_manager/manager.rs b/codex-rs/core/src/models_manager/manager.rs index ffd3dd9648..8b484ec847 100644 --- a/codex-rs/core/src/models_manager/manager.rs +++ b/codex-rs/core/src/models_manager/manager.rs @@ -103,36 +103,12 @@ impl ModelsManager { Ok(self.build_available_models(remote_models)) } - /// Determine whether a model supports personalities, with a local fallback when - /// remote metadata omits the instructions template. + /// Determine whether a model supports personalities based on remote metadata. pub fn supports_personality(&self, model: &str, config: &Config) -> bool { - let remote_supports = self - .try_get_remote_models(config) + self.try_get_remote_models(config) .ok() .and_then(|remote_models| remote_models.into_iter().find(|info| info.slug == model)) - .is_some_and(|info| info.supports_personality()); - if remote_supports { - return true; - } - - if self - .local_models - .iter() - .find(|preset| preset.model == model) - .is_some_and(|preset| preset.supports_personality) - { - return true; - } - - self.try_list_models(config) - .ok() - .and_then(|models| { - models - .into_iter() - .find(|preset| preset.model == model) - .map(|preset| preset.supports_personality) - }) - .unwrap_or(false) + .is_some_and(|info| info.supports_personality()) } // todo(aibrahim): should be visible to core only and sent on session_configured event @@ -174,14 +150,7 @@ impl ModelsManager { .await .into_iter() .find(|m| m.slug == model); - let model = if let Some(mut remote) = remote { - if !remote.supports_personality() && local.supports_personality() { - remote.model_instructions_template = local.model_instructions_template.clone(); - } - remote - } else { - local - }; + let model = remote.unwrap_or(local); model_info::with_config_overrides(model, config) } @@ -360,6 +329,12 @@ impl ModelsManager { } } + #[cfg(any(test, feature = "test-support"))] + /// Override remote model metadata for tests. + pub async fn set_remote_models_for_testing(&self, models: Vec) { + *self.remote_models.write().await = models; + } + #[cfg(any(test, feature = "test-support"))] /// Get model identifier without consulting remote state or cache. pub fn get_model_offline(model: Option<&str>) -> String { @@ -737,7 +712,7 @@ mod tests { } #[tokio::test] - async fn supports_personality_falls_back_to_local_presets() { + async fn supports_personality_requires_remote_metadata() { let codex_home = tempdir().expect("temp dir"); let mut config = ConfigBuilder::default() .codex_home(codex_home.path().to_path_buf()) @@ -751,7 +726,7 @@ mod tests { let manager = ModelsManager::with_provider(codex_home.path().to_path_buf(), auth_manager, provider); - // Remote metadata without personality messages should not disable local support. + // Remote metadata must explicitly include personality support. let mut remote = remote_model("gpt-5.2-codex", "Remote gpt-5.2-codex", 0); remote.model_instructions_template = Some(ModelInstructionsTemplate { template: "{{ personality_message }}".to_string(), @@ -759,9 +734,9 @@ mod tests { }); *manager.remote_models.write().await = vec![remote]; - assert!(manager.supports_personality("gpt-5.2-codex", &config)); + assert!(!manager.supports_personality("gpt-5.2-codex", &config)); let model = manager.get_model_info("gpt-5.2-codex", &config).await; - assert!(model.supports_personality()); + assert!(!model.supports_personality()); } #[test] diff --git a/codex-rs/core/src/models_manager/model_info.rs b/codex-rs/core/src/models_manager/model_info.rs index 6995768980..8f2f853d81 100644 --- a/codex-rs/core/src/models_manager/model_info.rs +++ b/codex-rs/core/src/models_manager/model_info.rs @@ -1,13 +1,8 @@ -use std::collections::BTreeMap; - -use codex_protocol::config_types::Personality; use codex_protocol::config_types::Verbosity; use codex_protocol::openai_models::ApplyPatchToolType; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::ModelInfo; -use codex_protocol::openai_models::ModelInstructionsTemplate; use codex_protocol::openai_models::ModelVisibility; -use codex_protocol::openai_models::PersonalityMessages; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::openai_models::TruncationMode; @@ -27,23 +22,9 @@ const GPT_5_2_INSTRUCTIONS: &str = include_str!("../../gpt_5_2_prompt.md"); const GPT_5_1_CODEX_MAX_INSTRUCTIONS: &str = include_str!("../../gpt-5.1-codex-max_prompt.md"); const GPT_5_2_CODEX_INSTRUCTIONS: &str = include_str!("../../gpt-5.2-codex_prompt.md"); -const GPT_5_2_CODEX_INSTRUCTIONS_TEMPLATE: &str = - include_str!("../../templates/model_instructions/gpt-5.2-codex_instructions_template.md"); -const PERSONALITY_FRIENDLY: &str = include_str!("../../templates/personalities/friendly.md"); -const PERSONALITY_PRAGMATIC: &str = include_str!("../../templates/personalities/pragmatic.md"); pub(crate) const CONTEXT_WINDOW_272K: i64 = 272_000; -fn gpt_5_2_codex_personality_template() -> ModelInstructionsTemplate { - ModelInstructionsTemplate { - template: GPT_5_2_CODEX_INSTRUCTIONS_TEMPLATE.to_string(), - personality_messages: Some(PersonalityMessages(BTreeMap::from([ - (Personality::Friendly, PERSONALITY_FRIENDLY.to_string()), - (Personality::Pragmatic, PERSONALITY_PRAGMATIC.to_string()), - ]))), - } -} - macro_rules! model_info { ( $slug:expr $(, $key:ident : $value:expr )* $(,)? @@ -179,7 +160,6 @@ pub(crate) fn find_model_info_for_slug(slug: &str) -> ModelInfo { model_info!( slug, base_instructions: GPT_5_2_CODEX_INSTRUCTIONS.to_string(), - model_instructions_template: Some(gpt_5_2_codex_personality_template()), apply_patch_tool_type: Some(ApplyPatchToolType::Freeform), shell_type: ConfigShellToolType::ShellCommand, supports_parallel_tool_calls: true, @@ -206,7 +186,6 @@ pub(crate) fn find_model_info_for_slug(slug: &str) -> ModelInfo { model_info!( slug, base_instructions: GPT_5_2_CODEX_INSTRUCTIONS.to_string(), - model_instructions_template: Some(gpt_5_2_codex_personality_template()), apply_patch_tool_type: Some(ApplyPatchToolType::Freeform), shell_type: ConfigShellToolType::ShellCommand, supports_parallel_tool_calls: true, diff --git a/codex-rs/core/tests/suite/personality.rs b/codex-rs/core/tests/suite/personality.rs index 7a27866569..918cb5da96 100644 --- a/codex-rs/core/tests/suite/personality.rs +++ b/codex-rs/core/tests/suite/personality.rs @@ -38,9 +38,6 @@ use tokio::time::sleep; use wiremock::BodyPrintLimit; use wiremock::MockServer; -const LOCAL_FRIENDLY_TEMPLATE: &str = - "You optimize for team morale and being a supportive teammate as much as code quality."; - fn sse_completed(id: &str) -> String { sse(vec![ev_response_created(id), ev_completed(id)]) } @@ -116,7 +113,7 @@ async fn user_turn_personality_none_does_not_add_update_message() -> anyhow::Res } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn config_personality_some_sets_instructions_template() -> anyhow::Result<()> { +async fn config_personality_some_ignores_without_remote_template() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -151,10 +148,12 @@ async fn config_personality_some_sets_instructions_template() -> anyhow::Result< let request = resp_mock.single_request(); let instructions_text = request.instructions_text(); + let model_info = + ModelsManager::construct_model_info_offline("exp-codex-personality", &test.config); assert!( - instructions_text.contains(LOCAL_FRIENDLY_TEMPLATE), - "expected personality update to include the local friendly template, got: {instructions_text:?}" + instructions_text == model_info.base_instructions, + "expected base instructions without a remote personality template, got: {instructions_text:?}" ); let developer_texts = request.message_input_texts("developer"); @@ -169,7 +168,7 @@ async fn config_personality_some_sets_instructions_template() -> anyhow::Result< } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn user_turn_personality_some_adds_update_message() -> anyhow::Result<()> { +async fn user_turn_personality_some_ignores_without_remote_template() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); let server = start_mock_server().await; @@ -246,18 +245,11 @@ async fn user_turn_personality_some_adds_update_message() -> anyhow::Result<()> .expect("expected personality update request"); let developer_texts = request.message_input_texts("developer"); - let personality_text = developer_texts - .iter() - .find(|text| text.contains("")) - .expect("expected personality update message in developer input"); - assert!( - personality_text.contains("The user has requested a new communication style."), - "expected personality update preamble, got {personality_text:?}" - ); - assert!( - personality_text.contains(LOCAL_FRIENDLY_TEMPLATE), - "expected personality update to include the local friendly template, got: {personality_text:?}" + !developer_texts + .iter() + .any(|text| text.contains("")), + "did not expect a personality update message without a remote template" ); Ok(()) @@ -292,10 +284,16 @@ async fn user_turn_personality_remote_model_template_includes_update_message() - base_instructions: "base instructions".to_string(), model_instructions_template: Some(ModelInstructionsTemplate { template: "Base instructions\n{{ personality_message }}\n".to_string(), - personality_messages: Some(PersonalityMessages(BTreeMap::from([( - Personality::Friendly, - remote_personality_message.to_string(), - )]))), + personality_messages: Some(PersonalityMessages(BTreeMap::from([ + ( + Personality::Friendly, + remote_personality_message.to_string(), + ), + ( + Personality::Pragmatic, + "Pragmatic from remote template".to_string(), + ), + ]))), }), supports_reasoning_summaries: false, support_verbosity: false, diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 1af2ff6b28..ad6bad1ae3 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -67,7 +67,10 @@ use codex_protocol::config_types::CollaborationMode; use codex_protocol::config_types::ModeKind; use codex_protocol::config_types::Personality; use codex_protocol::config_types::Settings; +use codex_protocol::openai_models::ModelInfo; +use codex_protocol::openai_models::ModelInstructionsTemplate; use codex_protocol::openai_models::ModelPreset; +use codex_protocol::openai_models::PersonalityMessages; use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::parse_command::ParsedCommand; use codex_protocol::plan_tool::PlanItemArg; @@ -82,8 +85,10 @@ use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; use insta::assert_snapshot; use pretty_assertions::assert_eq; +use serde_json::json; #[cfg(target_os = "windows")] use serial_test::serial; +use std::collections::BTreeMap; use std::collections::HashSet; use std::path::PathBuf; use tempfile::NamedTempFile; @@ -124,6 +129,58 @@ fn snapshot(percent: f64) -> RateLimitSnapshot { } } +fn personality_template() -> ModelInstructionsTemplate { + ModelInstructionsTemplate { + template: "Base instructions\n{{ personality_message }}\n".to_string(), + personality_messages: Some(PersonalityMessages(BTreeMap::from([ + (Personality::Friendly, "Friendly message".to_string()), + (Personality::Pragmatic, "Pragmatic message".to_string()), + ]))), + } +} + +fn remote_model_with_personality(slug: &str) -> ModelInfo { + let mut model: ModelInfo = serde_json::from_value(json!({ + "slug": slug, + "display_name": slug, + "description": format!("{slug} description"), + "default_reasoning_level": "medium", + "supported_reasoning_levels": [ + {"effort": "low", "description": "low"}, + {"effort": "medium", "description": "medium"} + ], + "shell_type": "shell_command", + "visibility": "list", + "supported_in_api": true, + "priority": 0, + "upgrade": null, + "base_instructions": "base instructions", + "supports_reasoning_summaries": false, + "support_verbosity": false, + "default_verbosity": null, + "apply_patch_tool_type": null, + "truncation_policy": {"mode": "bytes", "limit": 10_000}, + "supports_parallel_tool_calls": false, + "context_window": 128_000, + "experimental_supported_tools": [], + })) + .expect("valid model"); + model.model_instructions_template = Some(personality_template()); + model +} + +async fn enable_personality_support(chat: &mut ChatWidget, models: &[&str]) { + chat.set_feature_enabled(Feature::RemoteModels, true); + let remote_models = models + .iter() + .copied() + .map(remote_model_with_personality) + .collect(); + chat.models_manager + .set_remote_models_for_testing(remote_models) + .await; +} + #[tokio::test] async fn resumed_initial_messages_render_history() { let (mut chat, mut rx, _ops) = make_chatwidget_manual(None).await; @@ -1151,6 +1208,7 @@ fn session_configured_event_for(model: &str) -> Event { #[tokio::test] async fn personality_nudge_respects_hidden_notice() { let (mut chat, _rx, _) = make_chatwidget_manual(Some("bengalfox")).await; + enable_personality_support(&mut chat, &["bengalfox"]).await; chat.config.notices.hide_personality_nudge = Some(true); chat.handle_codex_event(session_configured_event_for("bengalfox")); @@ -1165,6 +1223,7 @@ async fn personality_nudge_respects_hidden_notice() { #[tokio::test] async fn personality_nudge_shows_once_and_hides_after_seen() { let (mut chat, mut rx, _) = make_chatwidget_manual(Some("bengalfox")).await; + enable_personality_support(&mut chat, &["bengalfox"]).await; chat.handle_codex_event(session_configured_event_for("bengalfox")); assert!(matches!( @@ -1200,6 +1259,7 @@ async fn personality_nudge_shows_once_and_hides_after_seen() { #[tokio::test] async fn personality_nudge_escape_does_not_persist_hidden() { let (mut chat, mut rx, _) = make_chatwidget_manual(Some("bengalfox")).await; + enable_personality_support(&mut chat, &["bengalfox"]).await; chat.handle_codex_event(session_configured_event_for("bengalfox")); assert!(matches!( @@ -1233,13 +1293,15 @@ async fn personality_nudge_escape_does_not_persist_hidden() { #[tokio::test] async fn gpt_52_codex_supports_personality_command() { - let (chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.2-codex")).await; + let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("gpt-5.2-codex")).await; + enable_personality_support(&mut chat, &["gpt-5.2-codex"]).await; assert!(chat.current_model_supports_personality()); } #[tokio::test] async fn personality_nudge_shows_on_model_switch_after_escape() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("bengalfox")).await; + enable_personality_support(&mut chat, &["bengalfox"]).await; chat.handle_codex_event(session_configured_event_for("bengalfox")); assert!(matches!( @@ -2551,6 +2613,7 @@ async fn collab_mode_enabling_keeps_custom_until_selected() { #[tokio::test] async fn user_turn_includes_personality_from_config() { let (mut chat, _rx, mut op_rx) = make_chatwidget_manual(Some("bengalfox")).await; + enable_personality_support(&mut chat, &["bengalfox"]).await; chat.thread_id = Some(ThreadId::new()); chat.set_personality_nudge_hidden(true); chat.set_model("bengalfox"); @@ -3115,6 +3178,7 @@ async fn model_selection_popup_snapshot() { #[tokio::test] async fn personality_selection_popup_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("bengalfox")).await; + enable_personality_support(&mut chat, &["bengalfox"]).await; chat.thread_id = Some(ThreadId::new()); chat.open_personality_popup(); @@ -3125,6 +3189,7 @@ async fn personality_selection_popup_snapshot() { #[tokio::test] async fn personality_nudge_popup_snapshot() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(Some("bengalfox")).await; + enable_personality_support(&mut chat, &["bengalfox"]).await; chat.handle_codex_event(session_configured_event_for("bengalfox")); let popup = render_bottom_popup(&chat, 80);