[codex] add developer instructions (#5897)
we are using developer instructions for code reviews, we need to pass them in cli as well.
This commit is contained in:
@@ -321,6 +321,10 @@ pub struct NewConversationParams {
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub base_instructions: Option<String>,
|
||||
|
||||
/// Developer instructions that will be sent as a `developer` role message.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub developer_instructions: Option<String>,
|
||||
|
||||
/// Prompt used during conversation compaction.
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
pub compact_prompt: Option<String>,
|
||||
@@ -1129,6 +1133,7 @@ mod tests {
|
||||
sandbox: None,
|
||||
config: None,
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
include_apply_patch_tool: None,
|
||||
},
|
||||
|
||||
@@ -1760,6 +1760,7 @@ async fn derive_config_from_params(
|
||||
sandbox: sandbox_mode,
|
||||
config: cli_overrides,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
compact_prompt,
|
||||
include_apply_patch_tool,
|
||||
} = params;
|
||||
@@ -1773,6 +1774,7 @@ async fn derive_config_from_params(
|
||||
model_provider,
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
compact_prompt,
|
||||
include_apply_patch_tool,
|
||||
include_view_image_tool: None,
|
||||
|
||||
@@ -44,7 +44,9 @@ async fn test_send_message_success() -> Result<()> {
|
||||
|
||||
// Start a conversation using the new wire API.
|
||||
let new_conv_id = mcp
|
||||
.send_new_conversation_request(NewConversationParams::default())
|
||||
.send_new_conversation_request(NewConversationParams {
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let new_conv_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
@@ -143,7 +145,10 @@ async fn test_send_message_raw_notifications_opt_in() -> Result<()> {
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let new_conv_id = mcp
|
||||
.send_new_conversation_request(NewConversationParams::default())
|
||||
.send_new_conversation_request(NewConversationParams {
|
||||
developer_instructions: Some("Use the test harness tools.".to_string()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let new_conv_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
@@ -177,6 +182,9 @@ async fn test_send_message_raw_notifications_opt_in() -> Result<()> {
|
||||
})
|
||||
.await?;
|
||||
|
||||
let developer = read_raw_response_item(&mut mcp, conversation_id).await;
|
||||
assert_developer_message(&developer, "Use the test harness tools.");
|
||||
|
||||
let instructions = read_raw_response_item(&mut mcp, conversation_id).await;
|
||||
assert_instructions_message(&instructions);
|
||||
|
||||
@@ -316,6 +324,21 @@ fn assert_instructions_message(item: &ResponseItem) {
|
||||
}
|
||||
}
|
||||
|
||||
fn assert_developer_message(item: &ResponseItem, expected_text: &str) {
|
||||
match item {
|
||||
ResponseItem::Message { role, content, .. } => {
|
||||
assert_eq!(role, "developer");
|
||||
let texts = content_texts(content);
|
||||
assert_eq!(
|
||||
texts,
|
||||
vec![expected_text],
|
||||
"expected developer instructions message, got {texts:?}"
|
||||
);
|
||||
}
|
||||
other => panic!("expected developer instructions message, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
fn assert_environment_message(item: &ResponseItem) {
|
||||
match item {
|
||||
ResponseItem::Message { role, content, .. } => {
|
||||
|
||||
@@ -112,6 +112,7 @@ use crate::tools::spec::ToolsConfig;
|
||||
use crate::tools::spec::ToolsConfigParams;
|
||||
use crate::turn_diff_tracker::TurnDiffTracker;
|
||||
use crate::unified_exec::UnifiedExecSessionManager;
|
||||
use crate::user_instructions::DeveloperInstructions;
|
||||
use crate::user_instructions::UserInstructions;
|
||||
use crate::user_notification::UserNotification;
|
||||
use crate::util::backoff;
|
||||
@@ -171,6 +172,7 @@ impl Codex {
|
||||
model: config.model.clone(),
|
||||
model_reasoning_effort: config.model_reasoning_effort,
|
||||
model_reasoning_summary: config.model_reasoning_summary,
|
||||
developer_instructions: config.developer_instructions.clone(),
|
||||
user_instructions,
|
||||
base_instructions: config.base_instructions.clone(),
|
||||
compact_prompt: config.compact_prompt.clone(),
|
||||
@@ -265,6 +267,7 @@ pub(crate) struct TurnContext {
|
||||
/// the model as well as sandbox policies are resolved against this path
|
||||
/// instead of `std::env::current_dir()`.
|
||||
pub(crate) cwd: PathBuf,
|
||||
pub(crate) developer_instructions: Option<String>,
|
||||
pub(crate) base_instructions: Option<String>,
|
||||
pub(crate) compact_prompt: Option<String>,
|
||||
pub(crate) user_instructions: Option<String>,
|
||||
@@ -303,6 +306,9 @@ pub(crate) struct SessionConfiguration {
|
||||
model_reasoning_effort: Option<ReasoningEffortConfig>,
|
||||
model_reasoning_summary: ReasoningSummaryConfig,
|
||||
|
||||
/// Developer instructions that supplement the base instructions.
|
||||
developer_instructions: Option<String>,
|
||||
|
||||
/// Model instructions that are appended to the base instructions.
|
||||
user_instructions: Option<String>,
|
||||
|
||||
@@ -417,6 +423,7 @@ impl Session {
|
||||
sub_id,
|
||||
client,
|
||||
cwd: session_configuration.cwd.clone(),
|
||||
developer_instructions: session_configuration.developer_instructions.clone(),
|
||||
base_instructions: session_configuration.base_instructions.clone(),
|
||||
compact_prompt: session_configuration.compact_prompt.clone(),
|
||||
user_instructions: session_configuration.user_instructions.clone(),
|
||||
@@ -991,7 +998,10 @@ impl Session {
|
||||
}
|
||||
|
||||
pub(crate) fn build_initial_context(&self, turn_context: &TurnContext) -> Vec<ResponseItem> {
|
||||
let mut items = Vec::<ResponseItem>::with_capacity(2);
|
||||
let mut items = Vec::<ResponseItem>::with_capacity(3);
|
||||
if let Some(developer_instructions) = turn_context.developer_instructions.as_deref() {
|
||||
items.push(DeveloperInstructions::new(developer_instructions.to_string()).into());
|
||||
}
|
||||
if let Some(user_instructions) = turn_context.user_instructions.as_deref() {
|
||||
items.push(UserInstructions::new(user_instructions.to_string()).into());
|
||||
}
|
||||
@@ -1674,6 +1684,7 @@ async fn spawn_review_thread(
|
||||
sub_id: sub_id.to_string(),
|
||||
client,
|
||||
tools_config,
|
||||
developer_instructions: None,
|
||||
user_instructions: None,
|
||||
base_instructions: Some(base_instructions.clone()),
|
||||
compact_prompt: parent_turn_context.compact_prompt.clone(),
|
||||
@@ -2511,6 +2522,7 @@ mod tests {
|
||||
model: config.model.clone(),
|
||||
model_reasoning_effort: config.model_reasoning_effort,
|
||||
model_reasoning_summary: config.model_reasoning_summary,
|
||||
developer_instructions: config.developer_instructions.clone(),
|
||||
user_instructions: config.user_instructions.clone(),
|
||||
base_instructions: config.base_instructions.clone(),
|
||||
compact_prompt: config.compact_prompt.clone(),
|
||||
@@ -2586,6 +2598,7 @@ mod tests {
|
||||
model: config.model.clone(),
|
||||
model_reasoning_effort: config.model_reasoning_effort,
|
||||
model_reasoning_summary: config.model_reasoning_summary,
|
||||
developer_instructions: config.developer_instructions.clone(),
|
||||
user_instructions: config.user_instructions.clone(),
|
||||
base_instructions: config.base_instructions.clone(),
|
||||
compact_prompt: config.compact_prompt.clone(),
|
||||
|
||||
@@ -128,6 +128,9 @@ pub struct Config {
|
||||
/// Base instructions override.
|
||||
pub base_instructions: Option<String>,
|
||||
|
||||
/// Developer instructions override injected as a separate message.
|
||||
pub developer_instructions: Option<String>,
|
||||
|
||||
/// Compact prompt override.
|
||||
pub compact_prompt: Option<String>,
|
||||
|
||||
@@ -543,6 +546,11 @@ pub struct ConfigToml {
|
||||
|
||||
/// System instructions.
|
||||
pub instructions: Option<String>,
|
||||
|
||||
/// Developer instructions inserted as a `developer` role message.
|
||||
#[serde(default)]
|
||||
pub developer_instructions: Option<String>,
|
||||
|
||||
/// Compact prompt used for history compaction.
|
||||
pub compact_prompt: Option<String>,
|
||||
|
||||
@@ -830,6 +838,7 @@ pub struct ConfigOverrides {
|
||||
pub config_profile: Option<String>,
|
||||
pub codex_linux_sandbox_exe: Option<PathBuf>,
|
||||
pub base_instructions: Option<String>,
|
||||
pub developer_instructions: Option<String>,
|
||||
pub compact_prompt: Option<String>,
|
||||
pub include_apply_patch_tool: Option<bool>,
|
||||
pub include_view_image_tool: Option<bool>,
|
||||
@@ -861,6 +870,7 @@ impl Config {
|
||||
config_profile: config_profile_key,
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
compact_prompt,
|
||||
include_apply_patch_tool: include_apply_patch_tool_override,
|
||||
include_view_image_tool: include_view_image_tool_override,
|
||||
@@ -1060,6 +1070,7 @@ impl Config {
|
||||
"experimental instructions file",
|
||||
)?;
|
||||
let base_instructions = base_instructions.or(file_base_instructions);
|
||||
let developer_instructions = developer_instructions.or(cfg.developer_instructions);
|
||||
|
||||
let experimental_compact_prompt_path = config_profile
|
||||
.experimental_compact_prompt_file
|
||||
@@ -1095,6 +1106,7 @@ impl Config {
|
||||
notify: cfg.notify,
|
||||
user_instructions,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
compact_prompt,
|
||||
// The config.toml omits "_mode" because it's a config file. However, "_mode"
|
||||
// is important in code to differentiate the mode from the store implementation.
|
||||
@@ -2886,6 +2898,7 @@ model_verbosity = "high"
|
||||
model_verbosity: None,
|
||||
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
forced_chatgpt_workspace_id: None,
|
||||
forced_login_method: None,
|
||||
@@ -2958,6 +2971,7 @@ model_verbosity = "high"
|
||||
model_verbosity: None,
|
||||
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
forced_chatgpt_workspace_id: None,
|
||||
forced_login_method: None,
|
||||
@@ -3045,6 +3059,7 @@ model_verbosity = "high"
|
||||
model_verbosity: None,
|
||||
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
forced_chatgpt_workspace_id: None,
|
||||
forced_login_method: None,
|
||||
@@ -3118,6 +3133,7 @@ model_verbosity = "high"
|
||||
model_verbosity: Some(Verbosity::High),
|
||||
chatgpt_base_url: "https://chatgpt.com/backend-api/".to_string(),
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
forced_chatgpt_workspace_id: None,
|
||||
forced_login_method: None,
|
||||
|
||||
@@ -40,3 +40,31 @@ impl From<UserInstructions> for ResponseItem {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq)]
|
||||
#[serde(rename = "developer_instructions", rename_all = "snake_case")]
|
||||
pub(crate) struct DeveloperInstructions {
|
||||
text: String,
|
||||
}
|
||||
|
||||
impl DeveloperInstructions {
|
||||
pub fn new<T: Into<String>>(text: T) -> Self {
|
||||
Self { text: text.into() }
|
||||
}
|
||||
|
||||
pub fn into_text(self) -> String {
|
||||
self.text
|
||||
}
|
||||
}
|
||||
|
||||
impl From<DeveloperInstructions> for ResponseItem {
|
||||
fn from(di: DeveloperInstructions) -> Self {
|
||||
ResponseItem::Message {
|
||||
id: None,
|
||||
role: "developer".to_string(),
|
||||
content: vec![ContentItem::InputText {
|
||||
text: di.into_text(),
|
||||
}],
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -58,6 +58,18 @@ fn assert_message_role(request_body: &serde_json::Value, role: &str) {
|
||||
assert_eq!(request_body["role"].as_str().unwrap(), role);
|
||||
}
|
||||
|
||||
#[expect(clippy::expect_used)]
|
||||
fn assert_message_equals(request_body: &serde_json::Value, text: &str) {
|
||||
let content = request_body["content"][0]["text"]
|
||||
.as_str()
|
||||
.expect("invalid message content");
|
||||
|
||||
assert_eq!(
|
||||
content, text,
|
||||
"expected message content '{content}' to equal '{text}'"
|
||||
);
|
||||
}
|
||||
|
||||
#[expect(clippy::expect_used)]
|
||||
fn assert_message_starts_with(request_body: &serde_json::Value, text: &str) {
|
||||
let content = request_body["content"][0]["text"]
|
||||
@@ -608,6 +620,64 @@ async fn includes_user_instructions_message_in_request() {
|
||||
assert_message_ends_with(&request_body["input"][1], "</environment_context>");
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn includes_developer_instructions_message_in_request() {
|
||||
skip_if_no_network!();
|
||||
let server = MockServer::start().await;
|
||||
|
||||
let resp_mock =
|
||||
responses::mount_sse_once_match(&server, path("/v1/responses"), sse_completed("resp1"))
|
||||
.await;
|
||||
|
||||
let model_provider = ModelProviderInfo {
|
||||
base_url: Some(format!("{}/v1", server.uri())),
|
||||
..built_in_model_providers()["openai"].clone()
|
||||
};
|
||||
|
||||
let codex_home = TempDir::new().unwrap();
|
||||
let mut config = load_default_config_for_test(&codex_home);
|
||||
config.model_provider = model_provider;
|
||||
config.user_instructions = Some("be nice".to_string());
|
||||
config.developer_instructions = Some("be useful".to_string());
|
||||
|
||||
let conversation_manager =
|
||||
ConversationManager::with_auth(CodexAuth::from_api_key("Test API Key"));
|
||||
let codex = conversation_manager
|
||||
.new_conversation(config)
|
||||
.await
|
||||
.expect("create new conversation")
|
||||
.conversation;
|
||||
|
||||
codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "hello".into(),
|
||||
}],
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TaskComplete(_))).await;
|
||||
|
||||
let request = resp_mock.single_request();
|
||||
let request_body = request.body_json();
|
||||
|
||||
assert!(
|
||||
!request_body["instructions"]
|
||||
.as_str()
|
||||
.unwrap()
|
||||
.contains("be nice")
|
||||
);
|
||||
assert_message_role(&request_body["input"][0], "developer");
|
||||
assert_message_equals(&request_body["input"][0], "be useful");
|
||||
assert_message_role(&request_body["input"][1], "user");
|
||||
assert_message_starts_with(&request_body["input"][1], "<user_instructions>");
|
||||
assert_message_ends_with(&request_body["input"][1], "</user_instructions>");
|
||||
assert_message_role(&request_body["input"][2], "user");
|
||||
assert_message_starts_with(&request_body["input"][2], "<environment_context>");
|
||||
assert_message_ends_with(&request_body["input"][2], "</environment_context>");
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn azure_responses_request_includes_store_and_reasoning_ids() {
|
||||
skip_if_no_network!();
|
||||
|
||||
@@ -174,6 +174,7 @@ pub async fn run_main(cli: Cli, codex_linux_sandbox_exe: Option<PathBuf>) -> any
|
||||
model_provider,
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
include_apply_patch_tool: None,
|
||||
include_view_image_tool: None,
|
||||
|
||||
@@ -50,6 +50,10 @@ pub struct CodexToolCallParam {
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub base_instructions: Option<String>,
|
||||
|
||||
/// Developer instructions that should be injected as a developer role message.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub developer_instructions: Option<String>,
|
||||
|
||||
/// Prompt used when compacting the conversation.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub compact_prompt: Option<String>,
|
||||
@@ -145,6 +149,7 @@ impl CodexToolCallParam {
|
||||
sandbox,
|
||||
config: cli_overrides,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
compact_prompt,
|
||||
} = self;
|
||||
|
||||
@@ -159,6 +164,7 @@ impl CodexToolCallParam {
|
||||
model_provider: None,
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions,
|
||||
developer_instructions,
|
||||
compact_prompt,
|
||||
include_apply_patch_tool: None,
|
||||
include_view_image_tool: None,
|
||||
@@ -294,6 +300,10 @@ mod tests {
|
||||
"description": "The set of instructions to use instead of the default ones.",
|
||||
"type": "string"
|
||||
},
|
||||
"developer-instructions": {
|
||||
"description": "Developer instructions that should be injected as a developer role message.",
|
||||
"type": "string"
|
||||
},
|
||||
"compact-prompt": {
|
||||
"description": "Prompt used when compacting the conversation.",
|
||||
"type": "string"
|
||||
|
||||
@@ -341,6 +341,7 @@ async fn codex_tool_passes_base_instructions() -> anyhow::Result<()> {
|
||||
.send_codex_tool_call(CodexToolCallParam {
|
||||
prompt: "How are you?".to_string(),
|
||||
base_instructions: Some("You are a helpful assistant.".to_string()),
|
||||
developer_instructions: Some("Foreshadow upcoming tool calls.".to_string()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
@@ -367,10 +368,28 @@ async fn codex_tool_passes_base_instructions() -> anyhow::Result<()> {
|
||||
);
|
||||
|
||||
let requests = server.received_requests().await.unwrap();
|
||||
let request = requests[0].body_json::<serde_json::Value>().unwrap();
|
||||
let request = requests[0].body_json::<serde_json::Value>()?;
|
||||
let instructions = request["messages"][0]["content"].as_str().unwrap();
|
||||
assert!(instructions.starts_with("You are a helpful assistant."));
|
||||
|
||||
let developer_msg = request["messages"]
|
||||
.as_array()
|
||||
.and_then(|messages| {
|
||||
messages
|
||||
.iter()
|
||||
.find(|msg| msg.get("role").and_then(|role| role.as_str()) == Some("developer"))
|
||||
})
|
||||
.unwrap();
|
||||
let developer_content = developer_msg
|
||||
.get("content")
|
||||
.and_then(|value| value.as_str())
|
||||
.unwrap();
|
||||
assert!(
|
||||
!developer_content.contains('<'),
|
||||
"expected developer instructions without XML tags, got `{developer_content}`"
|
||||
);
|
||||
assert_eq!(developer_content, "Foreshadow upcoming tool calls.");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -144,6 +144,7 @@ pub async fn run_main(
|
||||
config_profile: cli.config_profile.clone(),
|
||||
codex_linux_sandbox_exe,
|
||||
base_instructions: None,
|
||||
developer_instructions: None,
|
||||
compact_prompt: None,
|
||||
include_apply_patch_tool: None,
|
||||
include_view_image_tool: None,
|
||||
|
||||
Reference in New Issue
Block a user