mirror of
https://github.com/openai/codex.git
synced 2026-10-20 08:19:50 +00:00
Honor personality = "none" in model instructions (#32277)
## Why Model catalog instructions can include a baked-in `# Personality` section. An explicit `none` setting should omit that section instead of sending it as part of the model's base instructions. ## What changed - Pass the configured personality into the models manager. - When personality support is enabled and the setting is explicitly `none`, remove the `# Personality` section through the next level-one heading from catalog base instructions and instruction templates. - Preserve explicit `base_instructions` overrides and avoid warning when no personality was requested. ## Testing Added unit and integration coverage for section removal, heading boundaries, CRLF input, preserved configurations, and explicit base instructions. GitOrigin-RevId: 452c88d3ac6001c2ac7d4fef269cd75dc239fa61
This commit is contained in:
@@ -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_catalog: self.model_catalog.clone(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -259,6 +259,82 @@ 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.4", |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.4").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(()));
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use codex_protocol::config_types::Personality;
|
||||
use codex_protocol::openai_models::ModelsResponse;
|
||||
|
||||
#[derive(Debug, Clone, Default)]
|
||||
@@ -7,5 +8,6 @@ pub struct ModelsManagerConfig {
|
||||
pub tool_output_token_limit: Option<usize>,
|
||||
pub base_instructions: Option<String>,
|
||||
pub personality_enabled: bool,
|
||||
pub personality: Option<Personality>,
|
||||
pub model_catalog: Option<ModelsResponse>,
|
||||
}
|
||||
|
||||
@@ -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(context_window) = config.model_context_window {
|
||||
@@ -50,13 +52,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;
|
||||
|
||||
@@ -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<Personality>) -> ModelsManagerConfig {
|
||||
ModelsManagerConfig {
|
||||
personality_enabled: true,
|
||||
personality,
|
||||
..Default::default()
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn base_instruction_override_preserves_catalog_approval_messages() {
|
||||
let mut model = model_info_from_slug("unknown-model");
|
||||
@@ -65,6 +74,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");
|
||||
|
||||
@@ -465,14 +465,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()
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user