From f469f0bb6ec85848e4ecd450a10ffb42dd7bba35 Mon Sep 17 00:00:00 2001 From: Roy Han Date: Wed, 8 Jul 2026 14:19:37 -0700 Subject: [PATCH] Support harness-only personality none --- codex-rs/core/src/config/mod.rs | 1 + codex-rs/core/tests/suite/personality.rs | 78 +++++++++++++++++++ codex-rs/models-manager/src/config.rs | 2 + codex-rs/models-manager/src/model_info.rs | 55 ++++++++++++- .../models-manager/src/model_info_tests.rs | 78 +++++++++++++++++++ codex-rs/protocol/src/openai_models.rs | 17 ++-- 6 files changed, 222 insertions(+), 9 deletions(-) diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 8c0a165564db..8518c9e17723 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -1473,6 +1473,7 @@ impl Config { tool_output_token_limit: self.tool_output_token_limit, base_instructions: self.base_instructions.clone(), personality_enabled: self.features.enabled(Feature::Personality), + personality: self.personality, model_supports_reasoning_summaries: self.model_supports_reasoning_summaries, model_catalog: self.model_catalog.clone(), } diff --git a/codex-rs/core/tests/suite/personality.rs b/codex-rs/core/tests/suite/personality.rs index 88733b466674..0236820699c3 100644 --- a/codex-rs/core/tests/suite/personality.rs +++ b/codex-rs/core/tests/suite/personality.rs @@ -266,6 +266,84 @@ async fn config_personality_none_sends_no_personality() -> anyhow::Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_personality_none_strips_baked_personality_section() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + 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_info_override("gpt-5.3-codex", |model_info| { + model_info.base_instructions = "Base instructions\n# Personality\nBaked personality\n## Writing Style\nNested writing style\n# General\nGeneral instructions".to_string(); + model_info.model_messages = None; + }) + .with_config(|config| { + config + .features + .enable(Feature::Personality) + .expect("test config should allow feature update"); + config.personality = Some(Personality::None); + }); + let test = builder.build(&server).await?; + + test.codex + .submit(read_only_text_turn( + &test, + "hello", + test.session_configured.model.clone(), + test.config.permissions.approval_policy.value(), + )) + .await?; + + wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + assert_eq!( + resp_mock.single_request().instructions_text(), + "Base instructions\n# General\nGeneral instructions" + ); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn config_personality_none_preserves_explicit_base_instructions() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + + const CUSTOM_INSTRUCTIONS: &str = "Custom instructions\n# Personality\nThis must remain\n## Writing Style\nThis must also remain\n# General\nGeneral instructions"; + + 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.3-codex") + .with_config(|config| { + config + .features + .enable(Feature::Personality) + .expect("test config should allow feature update"); + config.personality = Some(Personality::None); + config.base_instructions = Some(CUSTOM_INSTRUCTIONS.to_string()); + }); + let test = builder.build(&server).await?; + + test.codex + .submit(read_only_text_turn( + &test, + "hello", + test.session_configured.model.clone(), + test.config.permissions.approval_policy.value(), + )) + .await?; + + wait_for_event(&test.codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + + assert_eq!( + resp_mock.single_request().instructions_text(), + CUSTOM_INSTRUCTIONS + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn default_personality_is_pragmatic_without_config_toml() -> anyhow::Result<()> { skip_if_no_network!(Ok(())); diff --git a/codex-rs/models-manager/src/config.rs b/codex-rs/models-manager/src/config.rs index b64add40fc50..b9e3dd621096 100644 --- a/codex-rs/models-manager/src/config.rs +++ b/codex-rs/models-manager/src/config.rs @@ -1,3 +1,4 @@ +use codex_protocol::config_types::Personality; use codex_protocol::openai_models::ModelsResponse; #[derive(Debug, Clone, Default)] @@ -7,6 +8,7 @@ pub struct ModelsManagerConfig { pub tool_output_token_limit: Option, pub base_instructions: Option, pub personality_enabled: bool, + pub personality: Option, pub model_supports_reasoning_summaries: Option, pub model_catalog: Option, } diff --git a/codex-rs/models-manager/src/model_info.rs b/codex-rs/models-manager/src/model_info.rs index 75d333efe382..49157375af19 100644 --- a/codex-rs/models-manager/src/model_info.rs +++ b/codex-rs/models-manager/src/model_info.rs @@ -1,3 +1,4 @@ +use codex_protocol::config_types::Personality; use codex_protocol::config_types::ReasoningSummary; use codex_protocol::openai_models::ConfigShellToolType; use codex_protocol::openai_models::ModelInfo; @@ -19,6 +20,7 @@ const LOCAL_FRIENDLY_TEMPLATE: &str = "You optimize for team morale and being a supportive teammate as much as code quality."; const LOCAL_PRAGMATIC_TEMPLATE: &str = "You are a deeply pragmatic, effective software engineer."; const PERSONALITY_PLACEHOLDER: &str = "{{ personality }}"; +const PERSONALITY_SECTION_HEADER: &str = "# Personality"; pub fn with_config_overrides(mut model: ModelInfo, config: &ModelsManagerConfig) -> ModelInfo { if let Some(supports_reasoning_summaries) = config.model_supports_reasoning_summaries @@ -55,13 +57,62 @@ pub fn with_config_overrides(mut model: ModelInfo, config: &ModelsManagerConfig) if let Some(base_instructions) = &config.base_instructions { model.base_instructions = base_instructions.clone(); clear_instruction_messages(&mut model); - } else if !config.personality_enabled { - clear_instruction_messages(&mut model); + } else { + if config.personality_enabled && config.personality == Some(Personality::None) { + model.base_instructions = strip_personality_section(model.base_instructions); + if let Some(instructions_template) = model + .model_messages + .as_mut() + .and_then(|messages| messages.instructions_template.as_mut()) + { + *instructions_template = + strip_personality_section(std::mem::take(instructions_template)); + } + } + if !config.personality_enabled { + clear_instruction_messages(&mut model); + } } model } +fn strip_personality_section(mut instructions: String) -> String { + let mut section_start = None; + let mut section_end = None; + let mut offset = 0; + + for line_with_ending in instructions.split_inclusive('\n') { + let line = match line_with_ending.strip_suffix('\n') { + Some(line) => line.strip_suffix('\r').unwrap_or(line), + None => line_with_ending, + }; + if section_start.is_some() { + if is_h1_heading(line) { + section_end = Some(offset); + break; + } + } else if line == PERSONALITY_SECTION_HEADER { + section_start = Some(offset); + } + offset += line_with_ending.len(); + } + + if let Some(section_start) = section_start { + let section_end = section_end.unwrap_or(instructions.len()); + instructions.replace_range(section_start..section_end, ""); + } + + instructions +} + +fn is_h1_heading(line: &str) -> bool { + let Some(rest) = line.strip_prefix('#') else { + return false; + }; + rest.is_empty() || rest.starts_with(' ') || rest.starts_with('\t') +} + fn clear_instruction_messages(model: &mut ModelInfo) { if let Some(model_messages) = &mut model.model_messages { model_messages.instructions_template = None; diff --git a/codex-rs/models-manager/src/model_info_tests.rs b/codex-rs/models-manager/src/model_info_tests.rs index 2240f0a39134..62c99cb4f8b0 100644 --- a/codex-rs/models-manager/src/model_info_tests.rs +++ b/codex-rs/models-manager/src/model_info_tests.rs @@ -1,8 +1,17 @@ use super::*; use crate::ModelsManagerConfig; +use codex_protocol::config_types::Personality; use codex_protocol::openai_models::ApprovalMessages; use pretty_assertions::assert_eq; +fn config_with_personality(personality: Option) -> ModelsManagerConfig { + ModelsManagerConfig { + personality_enabled: true, + personality, + ..Default::default() + } +} + #[test] fn reasoning_summaries_override_true_enables_support() { let model = model_info_from_slug("unknown-model"); @@ -107,6 +116,75 @@ fn disabled_personality_preserves_catalog_approval_messages() { ); } +#[test] +fn personality_none_strips_catalog_instruction_sources_through_the_next_h1() { + let cases = [ + ( + "Intro\n\n# Personality\n\nRemove me\n\n## Writing Style\n\nRemove me too\n\n# Safety\n\nKeep me", + "Intro\n\n# Safety\n\nKeep me", + ), + ("Intro\n\n# Personality\n\nRemove me", "Intro\n\n"), + ( + "Intro\n\n## Personality\n\nKeep me", + "Intro\n\n## Personality\n\nKeep me", + ), + ( + "Intro\n\n# Personality \n\nKeep me", + "Intro\n\n# Personality \n\nKeep me", + ), + ( + "Intro\r\n\r\n# Personality\r\n\r\nRemove me\r\n\r\n## Writing Style\r\n\r\nRemove me too\r\n\r\n# General\r\n\r\nKeep me", + "Intro\r\n\r\n# General\r\n\r\nKeep me", + ), + ]; + let config = config_with_personality(Some(Personality::None)); + + for (instructions, expected) in cases { + let mut model = model_info_from_slug("unknown-model"); + model.base_instructions = instructions.to_string(); + model.model_messages = Some(ModelMessages { + instructions_template: Some(instructions.to_string()), + instructions_variables: None, + approvals: None, + }); + + let updated = with_config_overrides(model, &config); + let instructions_template = updated + .model_messages + .as_ref() + .and_then(|messages| messages.instructions_template.as_deref()); + + assert_eq!( + (updated.base_instructions.as_str(), instructions_template), + (expected, Some(expected)) + ); + } +} + +#[test] +fn baked_personality_section_is_preserved_without_enabled_explicit_none() { + let instructions = "Intro\n# Personality\nKeep me\n# General\nKeep me too"; + let configs = [ + config_with_personality(/*personality*/ None), + config_with_personality(Some(Personality::Friendly)), + config_with_personality(Some(Personality::Pragmatic)), + ModelsManagerConfig { + personality: Some(Personality::None), + ..Default::default() + }, + ]; + + for config in configs { + let mut model = model_info_from_slug("unknown-model"); + model.base_instructions = instructions.to_string(); + + assert_eq!( + with_config_overrides(model, &config).base_instructions, + instructions + ); + } +} + #[test] fn model_context_window_override_clamps_to_max_context_window() { let mut model = model_info_from_slug("unknown-model"); diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 7e7c637aa0dd..797884bcb83d 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -466,14 +466,17 @@ impl ModelInfo { .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_messages is missing, falling back to base instructions." - ); - self.base_instructions.clone() } else { + match personality { + Some(personality @ (Personality::Friendly | Personality::Pragmatic)) => { + warn!( + model = %self.slug, + %personality, + "Model personality requested but model_messages is missing, falling back to base instructions." + ); + } + Some(Personality::None) | None => {} + } self.base_instructions.clone() } }