From e3ab0bd9739106b039ebffe040914d4aac0c6de6 Mon Sep 17 00:00:00 2001 From: Dylan Hurd Date: Fri, 30 Jan 2026 00:10:12 -0700 Subject: [PATCH] chore(personality) new schema with fallbacks (#10147) ## Summary Let's dial in this api contract in a bit more with more robust fallback behavior when model_instructions_template is false. Switches to a more explicit template / variables structure, with more fallbacks. ## Testing - [x] Adding unit tests - [x] Tested locally --- .../app-server/tests/common/models_cache.rs | 2 +- .../tests/suite/v2/thread_resume.rs | 13 +- .../app-server/tests/suite/v2/turn_start.rs | 9 +- .../codex-api/tests/models_integration.rs | 2 +- codex-rs/core/src/codex.rs | 52 ++- .../core/src/models_manager/model_info.rs | 55 +-- .../gpt-5.2-codex_instructions_template.md | 4 +- ...{friendly.md => gpt-5.2-codex_friendly.md} | 0 ...ragmatic.md => gpt-5.2-codex_pragmatic.md} | 1 + codex-rs/core/tests/suite/models_cache_ttl.rs | 2 +- codex-rs/core/tests/suite/personality.rs | 360 +++++++++++++++++- codex-rs/core/tests/suite/remote_models.rs | 6 +- codex-rs/protocol/src/openai_models.rs | 216 +++++++++-- 13 files changed, 607 insertions(+), 115 deletions(-) rename codex-rs/core/templates/personalities/{friendly.md => gpt-5.2-codex_friendly.md} (100%) rename codex-rs/core/templates/personalities/{pragmatic.md => gpt-5.2-codex_pragmatic.md} (99%) diff --git a/codex-rs/app-server/tests/common/models_cache.rs b/codex-rs/app-server/tests/common/models_cache.rs index 7a093dc7f..59fddc242 100644 --- a/codex-rs/app-server/tests/common/models_cache.rs +++ b/codex-rs/app-server/tests/common/models_cache.rs @@ -27,7 +27,7 @@ fn preset_to_info(preset: &ModelPreset, priority: i32) -> ModelInfo { priority, upgrade: preset.upgrade.as_ref().map(|u| u.into()), base_instructions: "base instructions".to_string(), - model_instructions_template: None, + model_messages: None, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, diff --git a/codex-rs/app-server/tests/suite/v2/thread_resume.rs b/codex-rs/app-server/tests/suite/v2/thread_resume.rs index af746b37b..6ad0a14b3 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_resume.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_resume.rs @@ -31,7 +31,7 @@ use tempfile::TempDir; use tokio::time::timeout; const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); -const DEFAULT_BASE_INSTRUCTIONS: &str = "You are Codex, based on GPT-5. You are running as a coding agent in the Codex CLI on a user's computer."; +const CODEX_5_2_INSTRUCTIONS_TEMPLATE_DEFAULT: &str = "You are Codex, a coding agent based on GPT-5. You and the user share the same workspace and collaborate to achieve the user's goals."; #[tokio::test] async fn thread_resume_returns_original_thread() -> Result<()> { @@ -368,7 +368,7 @@ async fn thread_resume_supports_history_and_overrides() -> Result<()> { } #[tokio::test] -async fn thread_resume_accepts_personality_override_v2() -> Result<()> { +async fn thread_resume_accepts_personality_override() -> Result<()> { skip_if_no_network!(Ok(())); let server = responses::start_mock_server().await; @@ -438,14 +438,14 @@ async fn thread_resume_accepts_personality_override_v2() -> Result<()> { let request = response_mock.single_request(); let developer_texts = request.message_input_texts("developer"); assert!( - !developer_texts + developer_texts .iter() .any(|text| text.contains("")), - "did not expect a personality update message in developer input, got {developer_texts:?}" + "expected a personality update message in developer input, got {developer_texts:?}" ); let instructions_text = request.instructions_text(); assert!( - instructions_text.contains(DEFAULT_BASE_INSTRUCTIONS), + instructions_text.contains(CODEX_5_2_INSTRUCTIONS_TEMPLATE_DEFAULT), "expected default base instructions from history, got {instructions_text:?}" ); @@ -459,7 +459,7 @@ fn create_config_toml(codex_home: &std::path::Path, server_uri: &str) -> std::io config_toml, format!( r#" -model = "mock-model" +model = "gpt-5.2-codex" approval_policy = "never" sandbox_mode = "read-only" @@ -467,6 +467,7 @@ model_provider = "mock_provider" [features] remote_models = false +personality = true [model_providers.mock_provider] name = "Mock provider for test" diff --git a/codex-rs/app-server/tests/suite/v2/turn_start.rs b/codex-rs/app-server/tests/suite/v2/turn_start.rs index 3abe73e2e..99eff9ecb 100644 --- a/codex-rs/app-server/tests/suite/v2/turn_start.rs +++ b/codex-rs/app-server/tests/suite/v2/turn_start.rs @@ -63,7 +63,7 @@ async fn turn_start_sends_originator_header() -> Result<()> { codex_home.path(), &server.uri(), "never", - &BTreeMap::default(), + &BTreeMap::from([(Feature::Personality, true)]), )?; let mut mcp = McpProcess::new(codex_home.path()).await?; @@ -138,7 +138,7 @@ async fn turn_start_emits_user_message_item_with_text_elements() -> Result<()> { codex_home.path(), &server.uri(), "never", - &BTreeMap::default(), + &BTreeMap::from([(Feature::Personality, true)]), )?; let mut mcp = McpProcess::new(codex_home.path()).await?; @@ -230,7 +230,7 @@ async fn turn_start_emits_notifications_and_accepts_model_override() -> Result<( codex_home.path(), &server.uri(), "never", - &BTreeMap::default(), + &BTreeMap::from([(Feature::Personality, true)]), )?; let mut mcp = McpProcess::new(codex_home.path()).await?; @@ -425,7 +425,7 @@ async fn turn_start_accepts_personality_override_v2() -> Result<()> { codex_home.path(), &server.uri(), "never", - &BTreeMap::default(), + &BTreeMap::from([(Feature::Personality, true)]), )?; let mut mcp = McpProcess::new(codex_home.path()).await?; @@ -473,6 +473,7 @@ async fn turn_start_accepts_personality_override_v2() -> Result<()> { if developer_texts.is_empty() { eprintln!("request body: {}", request.body_json()); } + assert!( developer_texts .iter() diff --git a/codex-rs/codex-api/tests/models_integration.rs b/codex-rs/codex-api/tests/models_integration.rs index b1c4f060b..c75c46a28 100644 --- a/codex-rs/codex-api/tests/models_integration.rs +++ b/codex-rs/codex-api/tests/models_integration.rs @@ -77,7 +77,7 @@ async fn models_client_hits_models_endpoint() { priority: 1, upgrade: None, base_instructions: "base instructions".to_string(), - model_instructions_template: None, + model_messages: None, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, diff --git a/codex-rs/core/src/codex.rs b/codex-rs/core/src/codex.rs index 1fb557218..250ea1ff4 100644 --- a/codex-rs/core/src/codex.rs +++ b/codex-rs/core/src/codex.rs @@ -1285,27 +1285,31 @@ impl Session { previous: Option<&Arc>, next: &TurnContext, ) -> Option { - let personality = next.personality?; - if let Some(prev) = previous - && prev.personality == Some(personality) - { + if !self.features.enabled(Feature::Personality) { return None; } - let model_info = next.client.get_model_info(); - let personality_message = Self::personality_message_for(&model_info, personality); + let previous = previous?; - personality_message.map(|personality_message| { - DeveloperInstructions::personality_spec_message(personality_message).into() - }) + // if a personality is specified and it's different from the previous one, build a personality update item + if let Some(personality) = next.personality + && next.personality != previous.personality + { + let model_info = next.client.get_model_info(); + let personality_message = Self::personality_message_for(&model_info, personality); + personality_message.map(|personality_message| { + DeveloperInstructions::personality_spec_message(personality_message).into() + }) + } else { + None + } } fn personality_message_for(model_info: &ModelInfo, personality: Personality) -> Option { model_info - .model_instructions_template + .model_messages .as_ref() - .and_then(|template| template.personality_messages.as_ref()) - .and_then(|messages| messages.0.get(&personality)) - .cloned() + .and_then(|spec| spec.get_personality_message(Some(personality))) + .filter(|message| !message.is_empty()) } fn build_collaboration_mode_update_item( @@ -1845,15 +1849,33 @@ impl Session { items.push(DeveloperInstructions::new(developer_instructions.to_string()).into()); } // Add developer instructions from collaboration_mode if they exist and are non-empty - let collaboration_mode = { + let (collaboration_mode, base_instructions) = { let state = self.state.lock().await; - state.session_configuration.collaboration_mode.clone() + ( + state.session_configuration.collaboration_mode.clone(), + state.session_configuration.base_instructions.clone(), + ) }; if let Some(collab_instructions) = DeveloperInstructions::from_collaboration_mode(&collaboration_mode) { items.push(collab_instructions.into()); } + if self.features.enabled(Feature::Personality) + && let Some(personality) = turn_context.personality + { + let model_info = turn_context.client.get_model_info(); + let has_baked_personality = model_info.supports_personality() + && base_instructions == model_info.get_model_instructions(Some(personality)); + if !has_baked_personality + && let Some(personality_message) = + Self::personality_message_for(&model_info, personality) + { + items.push( + DeveloperInstructions::personality_spec_message(personality_message).into(), + ); + } + } if let Some(user_instructions) = turn_context.user_instructions.as_deref() { items.push( UserInstructions { diff --git a/codex-rs/core/src/models_manager/model_info.rs b/codex-rs/core/src/models_manager/model_info.rs index b50f23274..eaab16000 100644 --- a/codex-rs/core/src/models_manager/model_info.rs +++ b/codex-rs/core/src/models_manager/model_info.rs @@ -1,19 +1,17 @@ -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::ModelInstructionsVariables; +use codex_protocol::openai_models::ModelMessages; 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; use codex_protocol::openai_models::TruncationPolicyConfig; use crate::config::Config; +use crate::features::Feature; use crate::truncate::approx_bytes_for_tokens; use tracing::warn; @@ -29,8 +27,11 @@ const GPT_5_1_CODEX_MAX_INSTRUCTIONS: &str = include_str!("../../gpt-5.1-codex-m 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"); + +const GPT_5_2_CODEX_PERSONALITY_FRIENDLY: &str = + include_str!("../../templates/personalities/gpt-5.2-codex_friendly.md"); +const GPT_5_2_CODEX_PERSONALITY_PRAGMATIC: &str = + include_str!("../../templates/personalities/gpt-5.2-codex_pragmatic.md"); pub(crate) const CONTEXT_WINDOW_272K: i64 = 272_000; @@ -54,7 +55,7 @@ macro_rules! model_info { priority: 99, upgrade: None, base_instructions: BASE_INSTRUCTIONS.to_string(), - model_instructions_template: None, + model_messages: None, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, @@ -100,8 +101,11 @@ pub(crate) fn with_config_overrides(mut model: ModelInfo, config: &Config) -> Mo if let Some(base_instructions) = &config.base_instructions { model.base_instructions = base_instructions.clone(); - model.model_instructions_template = None; + model.model_messages = None; + } else if !config.features.enabled(Feature::Personality) { + model.model_messages = None; } + model } @@ -169,15 +173,13 @@ 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(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(), - )]))), + model_messages: Some(ModelMessages { + instructions_template: Some(GPT_5_2_CODEX_INSTRUCTIONS_TEMPLATE.to_string()), + instructions_variables: Some(ModelInstructionsVariables { + personality_default: Some("".to_string()), + personality_friendly: Some(GPT_5_2_CODEX_PERSONALITY_FRIENDLY.to_string()), + personality_pragmatic: Some(GPT_5_2_CODEX_PERSONALITY_PRAGMATIC.to_string()), + }), }), apply_patch_tool_type: Some(ApplyPatchToolType::Freeform), shell_type: ConfigShellToolType::ShellCommand, @@ -213,15 +215,14 @@ pub(crate) fn find_model_info_for_slug(slug: &str) -> ModelInfo { truncation_policy: TruncationPolicyConfig::tokens(10_000), context_window: Some(CONTEXT_WINDOW_272K), supported_reasoning_levels: supported_reasoning_level_low_medium_high_xhigh(), - model_instructions_template: Some(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(), - )]))), + base_instructions: GPT_5_2_CODEX_INSTRUCTIONS.to_string(), + model_messages: Some(ModelMessages { + instructions_template: Some(GPT_5_2_CODEX_INSTRUCTIONS_TEMPLATE.to_string()), + instructions_variables: Some(ModelInstructionsVariables { + personality_default: Some("".to_string()), + personality_friendly: Some(GPT_5_2_CODEX_PERSONALITY_FRIENDLY.to_string()), + personality_pragmatic: Some(GPT_5_2_CODEX_PERSONALITY_PRAGMATIC.to_string()), + }), }), ) } else if slug.starts_with("gpt-5.1-codex-max") { diff --git a/codex-rs/core/templates/model_instructions/gpt-5.2-codex_instructions_template.md b/codex-rs/core/templates/model_instructions/gpt-5.2-codex_instructions_template.md index 6e9e53e36..530e825d5 100644 --- a/codex-rs/core/templates/model_instructions/gpt-5.2-codex_instructions_template.md +++ b/codex-rs/core/templates/model_instructions/gpt-5.2-codex_instructions_template.md @@ -1,8 +1,6 @@ You are Codex, a coding agent based on GPT-5. You and the user share the same workspace and collaborate to achieve the user's goals. -# Personality - -{{ personality_message }} +{{ personality }} ## Tone and style - Anything you say outside of tool use is shown to the user. Do not narrate abstractly; explain what you are doing and why, using plain language. diff --git a/codex-rs/core/templates/personalities/friendly.md b/codex-rs/core/templates/personalities/gpt-5.2-codex_friendly.md similarity index 100% rename from codex-rs/core/templates/personalities/friendly.md rename to codex-rs/core/templates/personalities/gpt-5.2-codex_friendly.md diff --git a/codex-rs/core/templates/personalities/pragmatic.md b/codex-rs/core/templates/personalities/gpt-5.2-codex_pragmatic.md similarity index 99% rename from codex-rs/core/templates/personalities/pragmatic.md rename to codex-rs/core/templates/personalities/gpt-5.2-codex_pragmatic.md index 0fd76898a..ca1738e42 100644 --- a/codex-rs/core/templates/personalities/pragmatic.md +++ b/codex-rs/core/templates/personalities/gpt-5.2-codex_pragmatic.md @@ -1,4 +1,5 @@ # Personality + You are a deeply pragmatic, effective software engineer. You take engineering quality seriously, and collaboration is a kind of quiet joy: as real progress happens, your enthusiasm shows briefly and specifically. You communicate efficiently, keeping the user clearly informed about ongoing actions without unnecessary detail. ## Values diff --git a/codex-rs/core/tests/suite/models_cache_ttl.rs b/codex-rs/core/tests/suite/models_cache_ttl.rs index 69506cffc..5756f46a6 100644 --- a/codex-rs/core/tests/suite/models_cache_ttl.rs +++ b/codex-rs/core/tests/suite/models_cache_ttl.rs @@ -175,7 +175,7 @@ fn test_remote_model(slug: &str, priority: i32) -> ModelInfo { priority, upgrade: None, base_instructions: "base instructions".to_string(), - model_instructions_template: None, + model_messages: None, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, diff --git a/codex-rs/core/tests/suite/personality.rs b/codex-rs/core/tests/suite/personality.rs index 7a2786656..79e0d36c6 100644 --- a/codex-rs/core/tests/suite/personality.rs +++ b/codex-rs/core/tests/suite/personality.rs @@ -9,10 +9,10 @@ use codex_core::protocol::SandboxPolicy; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::ModelInfo; -use codex_protocol::openai_models::ModelInstructionsTemplate; +use codex_protocol::openai_models::ModelInstructionsVariables; +use codex_protocol::openai_models::ModelMessages; use codex_protocol::openai_models::ModelVisibility; use codex_protocol::openai_models::ModelsResponse; -use codex_protocol::openai_models::PersonalityMessages; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::openai_models::ReasoningEffortPreset; use codex_protocol::openai_models::TruncationPolicyConfig; @@ -29,7 +29,6 @@ use core_test_support::skip_if_no_network; use core_test_support::test_codex::test_codex; use core_test_support::wait_for_event; use pretty_assertions::assert_eq; -use std::collections::BTreeMap; use std::sync::Arc; use tempfile::TempDir; use tokio::time::Duration; @@ -49,6 +48,7 @@ fn sse_completed(id: &str) -> String { async fn model_personality_does_not_mutate_base_instructions_without_template() { let codex_home = TempDir::new().expect("create temp dir"); let mut config = load_default_config_for_test(&codex_home).await; + config.features.enable(Feature::Personality); config.model_personality = Some(Personality::Friendly); let model_info = ModelsManager::construct_model_info_offline("gpt-5.1", &config); @@ -62,6 +62,7 @@ async fn model_personality_does_not_mutate_base_instructions_without_template() async fn base_instructions_override_disables_personality_template() { let codex_home = TempDir::new().expect("create temp dir"); let mut config = load_default_config_for_test(&codex_home).await; + config.features.enable(Feature::Personality); config.model_personality = Some(Personality::Friendly); config.base_instructions = Some("override instructions".to_string()); @@ -80,7 +81,12 @@ async fn user_turn_personality_none_does_not_add_update_message() -> anyhow::Res let server = start_mock_server().await; let resp_mock = mount_sse_once(&server, sse_completed("resp-1")).await; - let mut builder = test_codex().with_model("gpt-5.2-codex"); + let mut builder = test_codex() + .with_model("gpt-5.2-codex") + .with_config(|config| { + config.features.disable(Feature::RemoteModels); + config.features.enable(Feature::Personality); + }); let test = builder.build(&server).await?; test.codex @@ -122,10 +128,11 @@ async fn config_personality_some_sets_instructions_template() -> anyhow::Result< let server = start_mock_server().await; let resp_mock = mount_sse_once(&server, sse_completed("resp-1")).await; let mut builder = test_codex() - .with_model("exp-codex-personality") + .with_model("gpt-5.2-codex") .with_config(|config| { - config.model_personality = Some(Personality::Friendly); config.features.disable(Feature::RemoteModels); + config.features.enable(Feature::Personality); + config.model_personality = Some(Personality::Friendly); }); let test = builder.build(&server).await?; @@ -182,6 +189,7 @@ async fn user_turn_personality_some_adds_update_message() -> anyhow::Result<()> .with_model("exp-codex-personality") .with_config(|config| { config.features.disable(Feature::RemoteModels); + config.features.enable(Feature::Personality); }); let test = builder.build(&server).await?; @@ -263,6 +271,330 @@ async fn user_turn_personality_some_adds_update_message() -> anyhow::Result<()> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn instructions_uses_base_if_feature_disabled() -> anyhow::Result<()> { + let codex_home = TempDir::new().expect("create temp dir"); + let mut config = load_default_config_for_test(&codex_home).await; + config.features.disable(Feature::Personality); + config.model_personality = Some(Personality::Friendly); + + let model_info = ModelsManager::construct_model_info_offline("gpt-5.2-codex", &config); + assert_eq!( + model_info.get_model_instructions(config.model_personality), + model_info.base_instructions + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn user_turn_personality_skips_if_feature_disabled() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = start_mock_server().await; + let resp_mock = mount_sse_sequence( + &server, + vec![sse_completed("resp-1"), sse_completed("resp-2")], + ) + .await; + let mut builder = test_codex() + .with_model("exp-codex-personality") + .with_config(|config| { + config.features.disable(Feature::RemoteModels); + config.features.disable(Feature::Personality); + }); + let test = builder.build(&server).await?; + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "hello".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: test.config.approval_policy.value(), + sandbox_policy: SandboxPolicy::ReadOnly, + model: test.session_configured.model.clone(), + effort: test.config.model_reasoning_effort, + summary: ReasoningSummary::Auto, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + test.codex + .submit(Op::OverrideTurnContext { + cwd: None, + approval_policy: None, + sandbox_policy: None, + windows_sandbox_level: None, + model: None, + effort: None, + summary: None, + collaboration_mode: None, + personality: Some(Personality::Friendly), + }) + .await?; + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "hello".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: test.config.approval_policy.value(), + sandbox_policy: SandboxPolicy::ReadOnly, + model: test.session_configured.model.clone(), + effort: test.config.model_reasoning_effort, + summary: ReasoningSummary::Auto, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let requests = resp_mock.requests(); + assert_eq!(requests.len(), 2, "expected two requests"); + let request = requests + .last() + .expect("expected personality update request"); + + let developer_texts = request.message_input_texts("developer"); + let personality_text = developer_texts + .iter() + .find(|text| text.contains("")); + assert!( + personality_text.is_none(), + "expected no personality preamble, got {personality_text:?}" + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn ignores_remote_model_personality_if_remote_models_disabled() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = MockServer::builder() + .body_print_limit(BodyPrintLimit::Limited(80_000)) + .start() + .await; + + let remote_slug = "gpt-5.2-codex"; + let remote_personality_message = "Friendly from remote template"; + let remote_model = ModelInfo { + slug: remote_slug.to_string(), + display_name: "Remote personality test".to_string(), + description: Some("Remote model with personality template".to_string()), + default_reasoning_level: Some(ReasoningEffort::Medium), + supported_reasoning_levels: vec![ReasoningEffortPreset { + effort: ReasoningEffort::Medium, + description: ReasoningEffort::Medium.to_string(), + }], + shell_type: ConfigShellToolType::UnifiedExec, + visibility: ModelVisibility::List, + supported_in_api: true, + priority: 1, + upgrade: None, + base_instructions: "base instructions".to_string(), + model_messages: Some(ModelMessages { + instructions_template: Some( + "Base instructions\n{{ personality_message }}\n".to_string(), + ), + instructions_variables: Some(ModelInstructionsVariables { + personality_default: None, + personality_friendly: Some(remote_personality_message.to_string()), + personality_pragmatic: None, + }), + }), + supports_reasoning_summaries: false, + support_verbosity: false, + default_verbosity: None, + apply_patch_tool_type: None, + truncation_policy: TruncationPolicyConfig::bytes(10_000), + supports_parallel_tool_calls: false, + context_window: Some(128_000), + auto_compact_token_limit: None, + effective_context_window_percent: 95, + experimental_supported_tools: Vec::new(), + }; + + let _models_mock = mount_models_once( + &server, + ModelsResponse { + models: vec![remote_model], + }, + ) + .await; + + let resp_mock = mount_sse_once(&server, sse_completed("resp-1")).await; + + let mut builder = test_codex() + .with_auth(codex_core::CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(|config| { + config.features.disable(Feature::RemoteModels); + config.features.enable(Feature::Personality); + config.model = Some(remote_slug.to_string()); + config.model_personality = Some(Personality::Friendly); + }); + let test = builder.build(&server).await?; + + wait_for_model_available( + &test.thread_manager.get_models_manager(), + remote_slug, + &test.config, + ) + .await; + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "hello".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + model: remote_slug.to_string(), + effort: test.config.model_reasoning_effort, + summary: ReasoningSummary::Auto, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let request = resp_mock.single_request(); + let instructions_text = request.instructions_text(); + + assert!( + instructions_text.contains("You are Codex, a coding agent based on GPT-5. You and the user share the same workspace and collaborate to achieve the user's goals."), + "expected instructions to use the template instructions, got: {instructions_text:?}" + ); + assert!( + instructions_text.contains( + "You optimize for team morale and being a supportive teammate as much as code quality." + ), + "expected instructions to include the local friendly personality template, got: {instructions_text:?}" + ); + assert!( + !instructions_text.contains("{{ personality_message }}"), + "expected legacy personality placeholder to be replaced, got: {instructions_text:?}" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn remote_model_default_personality_instructions_with_feature() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + let server = MockServer::builder() + .body_print_limit(BodyPrintLimit::Limited(80_000)) + .start() + .await; + + let remote_slug = "codex-remote-default-personality"; + let default_personality_message = "Default from remote template"; + let remote_model = ModelInfo { + slug: remote_slug.to_string(), + display_name: "Remote default personality test".to_string(), + description: Some("Remote model with default personality template".to_string()), + default_reasoning_level: Some(ReasoningEffort::Medium), + supported_reasoning_levels: vec![ReasoningEffortPreset { + effort: ReasoningEffort::Medium, + description: ReasoningEffort::Medium.to_string(), + }], + shell_type: ConfigShellToolType::UnifiedExec, + visibility: ModelVisibility::List, + supported_in_api: true, + priority: 1, + upgrade: None, + base_instructions: "base instructions".to_string(), + model_messages: Some(ModelMessages { + instructions_template: Some("Base instructions\n{{ personality }}\n".to_string()), + instructions_variables: Some(ModelInstructionsVariables { + personality_default: Some(default_personality_message.to_string()), + personality_friendly: Some("Friendly variant".to_string()), + personality_pragmatic: Some("Pragmatic variant".to_string()), + }), + }), + supports_reasoning_summaries: false, + support_verbosity: false, + default_verbosity: None, + apply_patch_tool_type: None, + truncation_policy: TruncationPolicyConfig::bytes(10_000), + supports_parallel_tool_calls: false, + context_window: Some(128_000), + auto_compact_token_limit: None, + effective_context_window_percent: 95, + experimental_supported_tools: Vec::new(), + }; + + let _models_mock = mount_models_once( + &server, + ModelsResponse { + models: vec![remote_model], + }, + ) + .await; + + let resp_mock = mount_sse_once(&server, sse_completed("resp-1")).await; + + let mut builder = test_codex() + .with_auth(codex_core::CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(|config| { + config.features.enable(Feature::RemoteModels); + config.features.enable(Feature::Personality); + config.model = Some(remote_slug.to_string()); + }); + let test = builder.build(&server).await?; + + wait_for_model_available( + &test.thread_manager.get_models_manager(), + remote_slug, + &test.config, + ) + .await; + + test.codex + .submit(Op::UserTurn { + items: vec![UserInput::Text { + text: "hello".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + cwd: test.cwd_path().to_path_buf(), + approval_policy: AskForApproval::Never, + sandbox_policy: SandboxPolicy::ReadOnly, + model: remote_slug.to_string(), + effort: test.config.model_reasoning_effort, + summary: ReasoningSummary::Auto, + collaboration_mode: None, + personality: None, + }) + .await?; + + wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + let request = resp_mock.single_request(); + let instructions_text = request.instructions_text(); + + assert!( + instructions_text.contains(default_personality_message), + "expected instructions to include the remote default personality template, got: {instructions_text:?}" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn user_turn_personality_remote_model_template_includes_update_message() -> anyhow::Result<()> { @@ -290,12 +622,15 @@ async fn user_turn_personality_remote_model_template_includes_update_message() - priority: 1, upgrade: None, 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(), - )]))), + model_messages: Some(ModelMessages { + instructions_template: Some( + "Base instructions\n{{ personality_message }}\n".to_string(), + ), + instructions_variables: Some(ModelInstructionsVariables { + personality_default: None, + personality_friendly: Some(remote_personality_message.to_string()), + personality_pragmatic: None, + }), }), supports_reasoning_summaries: false, support_verbosity: false, @@ -327,6 +662,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() - .with_auth(codex_core::CodexAuth::create_dummy_chatgpt_auth_for_testing()) .with_config(|config| { config.features.enable(Feature::RemoteModels); + config.features.enable(Feature::Personality); config.model = Some("gpt-5.2-codex".to_string()); }); let test = builder.build(&server).await?; diff --git a/codex-rs/core/tests/suite/remote_models.rs b/codex-rs/core/tests/suite/remote_models.rs index 71374cb84..f59f73fd1 100644 --- a/codex-rs/core/tests/suite/remote_models.rs +++ b/codex-rs/core/tests/suite/remote_models.rs @@ -79,7 +79,7 @@ async fn remote_models_remote_model_uses_unified_exec() -> Result<()> { priority: 1, upgrade: None, base_instructions: "base instructions".to_string(), - model_instructions_template: None, + model_messages: None, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, @@ -316,7 +316,7 @@ async fn remote_models_apply_remote_base_instructions() -> Result<()> { priority: 1, upgrade: None, base_instructions: remote_base.to_string(), - model_instructions_template: None, + model_messages: None, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, @@ -790,7 +790,7 @@ fn test_remote_model_with_policy( priority, upgrade: None, base_instructions: "base instructions".to_string(), - model_instructions_template: None, + model_messages: None, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 3f29484ff..9a940539d 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -1,4 +1,3 @@ -use std::collections::BTreeMap; use std::collections::HashMap; use std::collections::HashSet; @@ -14,7 +13,7 @@ use ts_rs::TS; use crate::config_types::Personality; use crate::config_types::Verbosity; -const PERSONALITY_PLACEHOLDER: &str = "{{ personality_message }}"; +const PERSONALITY_PLACEHOLDER: &str = "{{ personality }}"; /// See https://platform.openai.com/docs/guides/reasoning?api-mode=responses#get-started-with-reasoning #[derive( @@ -189,7 +188,7 @@ pub struct ModelInfo { pub upgrade: Option, pub base_instructions: String, #[serde(default, skip_serializing_if = "Option::is_none")] - pub model_instructions_template: Option, + pub model_messages: Option, pub supports_reasoning_summaries: bool, pub support_verbosity: bool, pub default_verbosity: Option, @@ -218,26 +217,25 @@ impl ModelInfo { } pub fn supports_personality(&self) -> bool { - self.model_instructions_template + self.model_messages .as_ref() - .is_some_and(ModelInstructionsTemplate::supports_personality) + .is_some_and(ModelMessages::supports_personality) } pub fn get_model_instructions(&self, personality: Option) -> String { - if let Some(personality) = personality - && let Some(template) = &self.model_instructions_template - && template.has_personality_placeholder() - && let Some(personality_messages) = &template.personality_messages - && let Some(personality_message) = personality_messages.0.get(&personality) + if let Some(model_messages) = &self.model_messages + && let Some(template) = &model_messages.instructions_template { - template - .template - .replace(PERSONALITY_PLACEHOLDER, personality_message.as_str()) + // if we have a template, always use it + let personality_message = model_messages + .get_personality_message(personality) + .unwrap_or_default(); + template.replace(PERSONALITY_PLACEHOLDER, personality_message.as_str()) } else if let Some(personality) = personality { warn!( model = %self.slug, %personality, - "Model personality requested but model_instructions_template is invalid, falling back to base instructions." + "Model personality requested but model_messages is missing, falling back to base instructions." ); self.base_instructions.clone() } else { @@ -246,31 +244,62 @@ impl ModelInfo { } } -/// A strongly-typed template for assembling model instructions. If populated and valid, will override -/// base_instructions. +/// A strongly-typed template for assembling model instructions and developer messages. If +/// instructions_* is populated and valid, it will override base_instructions. #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] -pub struct ModelInstructionsTemplate { - pub template: String, - pub personality_messages: Option, +pub struct ModelMessages { + pub instructions_template: Option, + pub instructions_variables: Option, } -impl ModelInstructionsTemplate { +impl ModelMessages { fn has_personality_placeholder(&self) -> bool { - self.template.contains(PERSONALITY_PLACEHOLDER) + self.instructions_template + .as_ref() + .map(|spec| spec.contains(PERSONALITY_PLACEHOLDER)) + .unwrap_or(false) } fn supports_personality(&self) -> bool { self.has_personality_placeholder() - && self.personality_messages.as_ref().is_some_and(|messages| { - Personality::iter().all(|personality| messages.0.contains_key(&personality)) - }) + && self + .instructions_variables + .as_ref() + .is_some_and(ModelInstructionsVariables::is_complete) + } + + pub fn get_personality_message(&self, personality: Option) -> Option { + self.instructions_variables + .as_ref() + .and_then(|variables| variables.get_personality_message(personality)) } } -// serializes as a dictionary from personality to message -#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, TS, JsonSchema)] -#[serde(transparent)] -pub struct PersonalityMessages(pub BTreeMap); +#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] +pub struct ModelInstructionsVariables { + pub personality_default: Option, + pub personality_friendly: Option, + pub personality_pragmatic: Option, +} + +impl ModelInstructionsVariables { + pub fn is_complete(&self) -> bool { + self.personality_default.is_some() + && self.personality_friendly.is_some() + && self.personality_pragmatic.is_some() + } + + pub fn get_personality_message(&self, personality: Option) -> Option { + if let Some(personality) = personality { + match personality { + Personality::Friendly => self.personality_friendly.clone(), + Personality::Pragmatic => self.personality_pragmatic.clone(), + } + } else { + self.personality_default.clone() + } + } +} #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] pub struct ModelInfoUpgrade { @@ -407,7 +436,7 @@ mod tests { use super::*; use pretty_assertions::assert_eq; - fn test_model(template: Option) -> ModelInfo { + fn test_model(spec: Option) -> ModelInfo { ModelInfo { slug: "test-model".to_string(), display_name: "Test Model".to_string(), @@ -420,7 +449,7 @@ mod tests { priority: 1, upgrade: None, base_instructions: "base".to_string(), - model_instructions_template: template, + model_messages: spec, supports_reasoning_summaries: false, support_verbosity: false, default_verbosity: None, @@ -434,18 +463,19 @@ mod tests { } } - fn personality_messages() -> PersonalityMessages { - PersonalityMessages(BTreeMap::from([( - Personality::Friendly, - "friendly".to_string(), - )])) + fn personality_variables() -> ModelInstructionsVariables { + ModelInstructionsVariables { + personality_default: Some("default".to_string()), + personality_friendly: Some("friendly".to_string()), + personality_pragmatic: Some("pragmatic".to_string()), + } } #[test] fn get_model_instructions_uses_template_when_placeholder_present() { - let model = test_model(Some(ModelInstructionsTemplate { - template: "Hello {{ personality_message }}".to_string(), - personality_messages: Some(personality_messages()), + let model = test_model(Some(ModelMessages { + instructions_template: Some("Hello {{ personality }}".to_string()), + instructions_variables: Some(personality_variables()), })); let instructions = model.get_model_instructions(Some(Personality::Friendly)); @@ -454,14 +484,116 @@ mod tests { } #[test] - fn get_model_instructions_falls_back_when_placeholder_missing() { - let model = test_model(Some(ModelInstructionsTemplate { - template: "Hello there".to_string(), - personality_messages: Some(personality_messages()), + fn get_model_instructions_always_strips_placeholder() { + let model = test_model(Some(ModelMessages { + instructions_template: Some("Hello\n{{ personality }}".to_string()), + instructions_variables: Some(ModelInstructionsVariables { + personality_default: None, + personality_friendly: Some("friendly".to_string()), + personality_pragmatic: None, + }), + })); + assert_eq!( + model.get_model_instructions(Some(Personality::Friendly)), + "Hello\nfriendly" + ); + assert_eq!( + model.get_model_instructions(Some(Personality::Pragmatic)), + "Hello\n" + ); + assert_eq!(model.get_model_instructions(None), "Hello\n"); + + let model_no_personality = test_model(Some(ModelMessages { + instructions_template: Some("Hello\n{{ personality }}".to_string()), + instructions_variables: Some(ModelInstructionsVariables { + personality_default: None, + personality_friendly: None, + personality_pragmatic: None, + }), + })); + assert_eq!( + model_no_personality.get_model_instructions(Some(Personality::Friendly)), + "Hello\n" + ); + assert_eq!( + model_no_personality.get_model_instructions(Some(Personality::Pragmatic)), + "Hello\n" + ); + assert_eq!(model_no_personality.get_model_instructions(None), "Hello\n"); + } + + #[test] + fn get_model_instructions_falls_back_when_template_is_missing() { + let model = test_model(Some(ModelMessages { + instructions_template: None, + instructions_variables: Some(ModelInstructionsVariables { + personality_default: None, + personality_friendly: None, + personality_pragmatic: None, + }), })); let instructions = model.get_model_instructions(Some(Personality::Friendly)); assert_eq!(instructions, "base"); } + + #[test] + fn get_personality_message_returns_default_when_personality_is_none() { + let personality_template = personality_variables(); + assert_eq!( + personality_template.get_personality_message(None), + Some("default".to_string()) + ); + } + + #[test] + fn get_personality_message() { + let personality_variables = personality_variables(); + assert_eq!( + personality_variables.get_personality_message(Some(Personality::Friendly)), + Some("friendly".to_string()) + ); + assert_eq!( + personality_variables.get_personality_message(Some(Personality::Pragmatic)), + Some("pragmatic".to_string()) + ); + assert_eq!( + personality_variables.get_personality_message(None), + Some("default".to_string()) + ); + + let personality_variables = ModelInstructionsVariables { + personality_default: Some("default".to_string()), + personality_friendly: None, + personality_pragmatic: None, + }; + assert_eq!( + personality_variables.get_personality_message(Some(Personality::Friendly)), + None + ); + assert_eq!( + personality_variables.get_personality_message(Some(Personality::Pragmatic)), + None + ); + assert_eq!( + personality_variables.get_personality_message(None), + Some("default".to_string()) + ); + + let personality_variables = ModelInstructionsVariables { + personality_default: None, + personality_friendly: Some("friendly".to_string()), + personality_pragmatic: Some("pragmatic".to_string()), + }; + assert_eq!( + personality_variables.get_personality_message(Some(Personality::Friendly)), + Some("friendly".to_string()) + ); + assert_eq!( + personality_variables.get_personality_message(Some(Personality::Pragmatic)), + Some("pragmatic".to_string()) + ); + assert_eq!(personality_variables.get_personality_message(None), None); + } }