mirror of
https://github.com/openai/codex.git
synced 2026-04-24 14:45:27 +00:00
fix(core) switching model appends model instructions (#10651)
## Summary When switching models, we should append the instructions of the new model to the conversation as a developer message. ## Test - [x] Adds a unit test
This commit is contained in:
@@ -1460,6 +1460,9 @@ impl Session {
|
||||
return None;
|
||||
}
|
||||
let previous = previous?;
|
||||
if next.model_info.slug != previous.model_info.slug {
|
||||
return None;
|
||||
}
|
||||
|
||||
// if a personality is specified and it's different from the previous one, build a personality update item
|
||||
if let Some(personality) = next.personality
|
||||
@@ -1498,6 +1501,24 @@ impl Session {
|
||||
}
|
||||
}
|
||||
|
||||
fn build_model_instructions_update_item(
|
||||
&self,
|
||||
previous: Option<&Arc<TurnContext>>,
|
||||
next: &TurnContext,
|
||||
) -> Option<ResponseItem> {
|
||||
let prev = previous?;
|
||||
if prev.model_info.slug == next.model_info.slug {
|
||||
return None;
|
||||
}
|
||||
|
||||
let model_instructions = next.model_info.get_model_instructions(next.personality);
|
||||
if model_instructions.is_empty() {
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(DeveloperInstructions::model_switch_message(model_instructions).into())
|
||||
}
|
||||
|
||||
fn build_settings_update_items(
|
||||
&self,
|
||||
previous_context: Option<&Arc<TurnContext>>,
|
||||
@@ -1519,6 +1540,11 @@ impl Session {
|
||||
{
|
||||
update_items.push(collaboration_mode_item);
|
||||
}
|
||||
if let Some(model_instructions_item) =
|
||||
self.build_model_instructions_update_item(previous_context, current_context)
|
||||
{
|
||||
update_items.push(model_instructions_item);
|
||||
}
|
||||
if let Some(personality_item) =
|
||||
self.build_personality_update_item(previous_context, current_context)
|
||||
{
|
||||
|
||||
@@ -705,6 +705,7 @@ async fn empty_collaboration_instructions_are_ignored() -> Result<()> {
|
||||
.await;
|
||||
|
||||
let test = test_codex().build(&server).await?;
|
||||
let current_model = test.session_configured.model.clone();
|
||||
|
||||
test.codex
|
||||
.submit(Op::OverrideTurnContext {
|
||||
@@ -715,7 +716,14 @@ async fn empty_collaboration_instructions_are_ignored() -> Result<()> {
|
||||
model: None,
|
||||
effort: None,
|
||||
summary: None,
|
||||
collaboration_mode: Some(collab_mode_with_instructions(Some(""))),
|
||||
collaboration_mode: Some(CollaborationMode {
|
||||
mode: ModeKind::Default,
|
||||
settings: Settings {
|
||||
model: current_model,
|
||||
reasoning_effort: None,
|
||||
developer_instructions: Some("".to_string()),
|
||||
},
|
||||
}),
|
||||
personality: None,
|
||||
})
|
||||
.await?;
|
||||
|
||||
@@ -84,6 +84,7 @@ mod live_cli;
|
||||
mod live_reload;
|
||||
mod model_info_overrides;
|
||||
mod model_overrides;
|
||||
mod model_switching;
|
||||
mod model_tools;
|
||||
mod models_cache_ttl;
|
||||
mod models_etag_responses;
|
||||
|
||||
192
codex-rs/core/tests/suite/model_switching.rs
Normal file
192
codex-rs/core/tests/suite/model_switching.rs
Normal file
@@ -0,0 +1,192 @@
|
||||
use anyhow::Result;
|
||||
use codex_core::config::types::Personality;
|
||||
use codex_core::features::Feature;
|
||||
use codex_core::protocol::AskForApproval;
|
||||
use codex_core::protocol::EventMsg;
|
||||
use codex_core::protocol::Op;
|
||||
use codex_core::protocol::SandboxPolicy;
|
||||
use codex_protocol::config_types::ReasoningSummary;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use core_test_support::responses::mount_sse_sequence;
|
||||
use core_test_support::responses::sse_completed;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
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;
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn model_change_appends_model_instructions_developer_message() -> 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("gpt-5.2-codex");
|
||||
let test = builder.build(&server).await?;
|
||||
let next_model = "gpt-5.1-codex-max";
|
||||
|
||||
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: 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: Some(next_model.to_string()),
|
||||
effort: None,
|
||||
summary: None,
|
||||
collaboration_mode: None,
|
||||
personality: None,
|
||||
})
|
||||
.await?;
|
||||
|
||||
test.codex
|
||||
.submit(Op::UserTurn {
|
||||
items: vec![UserInput::Text {
|
||||
text: "switch models".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: next_model.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 requests = resp_mock.requests();
|
||||
assert_eq!(requests.len(), 2, "expected two model requests");
|
||||
|
||||
let second_request = requests.last().expect("expected second request");
|
||||
let developer_texts = second_request.message_input_texts("developer");
|
||||
let model_switch_text = developer_texts
|
||||
.iter()
|
||||
.find(|text| text.contains("<model_switch>"))
|
||||
.expect("expected model switch message in developer input");
|
||||
assert!(
|
||||
model_switch_text.contains("The user was previously using a different model."),
|
||||
"expected model switch preamble, got: {model_switch_text:?}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn model_and_personality_change_only_appends_model_instructions() -> 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("gpt-5.2-codex")
|
||||
.with_config(|config| {
|
||||
config.features.enable(Feature::Personality);
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
let next_model = "exp-codex-personality";
|
||||
|
||||
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: 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: Some(next_model.to_string()),
|
||||
effort: None,
|
||||
summary: None,
|
||||
collaboration_mode: None,
|
||||
personality: Some(Personality::Pragmatic),
|
||||
})
|
||||
.await?;
|
||||
|
||||
test.codex
|
||||
.submit(Op::UserTurn {
|
||||
items: vec![UserInput::Text {
|
||||
text: "switch model and personality".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: next_model.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 requests = resp_mock.requests();
|
||||
assert_eq!(requests.len(), 2, "expected two model requests");
|
||||
|
||||
let second_request = requests.last().expect("expected second request");
|
||||
let developer_texts = second_request.message_input_texts("developer");
|
||||
assert!(
|
||||
developer_texts
|
||||
.iter()
|
||||
.any(|text| text.contains("<model_switch>")),
|
||||
"expected model switch message when model changes"
|
||||
);
|
||||
assert!(
|
||||
!developer_texts
|
||||
.iter()
|
||||
.any(|text| text.contains("<personality_spec>")),
|
||||
"did not expect personality update message when model changed in same turn"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -882,7 +882,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() -
|
||||
cwd: test.cwd_path().to_path_buf(),
|
||||
approval_policy: AskForApproval::Never,
|
||||
sandbox_policy: SandboxPolicy::ReadOnly,
|
||||
model: test.session_configured.model.clone(),
|
||||
model: remote_slug.to_string(),
|
||||
effort: test.config.model_reasoning_effort,
|
||||
summary: ReasoningSummary::Auto,
|
||||
collaboration_mode: None,
|
||||
@@ -898,7 +898,7 @@ async fn user_turn_personality_remote_model_template_includes_update_message() -
|
||||
approval_policy: None,
|
||||
sandbox_policy: None,
|
||||
windows_sandbox_level: None,
|
||||
model: Some(remote_slug.to_string()),
|
||||
model: None,
|
||||
effort: None,
|
||||
summary: None,
|
||||
collaboration_mode: None,
|
||||
|
||||
@@ -384,7 +384,7 @@ async fn overrides_turn_context_but_keeps_cached_prefix_and_key_constant() -> an
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
sandbox_policy: Some(new_policy.clone()),
|
||||
windows_sandbox_level: None,
|
||||
model: Some("o3".to_string()),
|
||||
model: None,
|
||||
effort: Some(Some(ReasoningEffort::High)),
|
||||
summary: Some(ReasoningSummary::Detailed),
|
||||
collaboration_mode: None,
|
||||
@@ -676,9 +676,21 @@ async fn per_turn_overrides_keep_cached_prefix_and_key_constant() -> anyhow::Res
|
||||
expected_permissions_msg_2, expected_permissions_msg,
|
||||
"expected updated permissions message after per-turn override"
|
||||
);
|
||||
let expected_model_switch_msg = body2["input"][body1_input.len() + 2].clone();
|
||||
assert_eq!(
|
||||
expected_model_switch_msg["role"].as_str(),
|
||||
Some("developer")
|
||||
);
|
||||
assert!(
|
||||
expected_model_switch_msg["content"][0]["text"]
|
||||
.as_str()
|
||||
.is_some_and(|text| text.contains("<model_switch>")),
|
||||
"expected model switch message after model override: {expected_model_switch_msg:?}"
|
||||
);
|
||||
let mut expected_body2 = body1_input.to_vec();
|
||||
expected_body2.push(expected_env_msg_2);
|
||||
expected_body2.push(expected_permissions_msg_2);
|
||||
expected_body2.push(expected_model_switch_msg);
|
||||
expected_body2.push(expected_user_message_2);
|
||||
assert_eq!(body2["input"], serde_json::Value::Array(expected_body2));
|
||||
|
||||
@@ -892,6 +904,17 @@ async fn send_user_turn_with_changes_sends_environment_context() -> anyhow::Resu
|
||||
expected_permissions_msg_2, expected_permissions_msg,
|
||||
"expected updated permissions message after policy change"
|
||||
);
|
||||
let expected_model_switch_msg = body2["input"][body1_input.len() + 1].clone();
|
||||
assert_eq!(
|
||||
expected_model_switch_msg["role"].as_str(),
|
||||
Some("developer")
|
||||
);
|
||||
assert!(
|
||||
expected_model_switch_msg["content"][0]["text"]
|
||||
.as_str()
|
||||
.is_some_and(|text| text.contains("<model_switch>")),
|
||||
"expected model switch message after model override: {expected_model_switch_msg:?}"
|
||||
);
|
||||
let expected_user_message_2 = text_user_input("hello 2".to_string());
|
||||
let expected_input_2 = serde_json::Value::Array(vec![
|
||||
expected_permissions_msg,
|
||||
@@ -899,6 +922,7 @@ async fn send_user_turn_with_changes_sends_environment_context() -> anyhow::Resu
|
||||
expected_env_msg_1,
|
||||
expected_user_message_1,
|
||||
expected_permissions_msg_2,
|
||||
expected_model_switch_msg,
|
||||
expected_user_message_2,
|
||||
]);
|
||||
assert_eq!(body2["input"], expected_input_2);
|
||||
|
||||
@@ -274,6 +274,12 @@ impl DeveloperInstructions {
|
||||
Self { text }
|
||||
}
|
||||
|
||||
pub fn model_switch_message(model_instructions: String) -> Self {
|
||||
DeveloperInstructions::new(format!(
|
||||
"<model_switch>\nThe user was previously using a different model. Please continue the conversation according to the following instructions:\n\n{model_instructions}\n</model_switch>"
|
||||
))
|
||||
}
|
||||
|
||||
pub fn personality_spec_message(spec: String) -> Self {
|
||||
let message = format!(
|
||||
"<personality_spec> The user has requested a new communication style. Future messages should adhere to the following personality: \n{spec} </personality_spec>"
|
||||
|
||||
Reference in New Issue
Block a user