Compare commits

...

3 Commits

Author SHA1 Message Date
Ahmed Ibrahim
2318b4f282 codex: keep test developer instructions at config root (#16976) 2026-04-06 21:43:08 -07:00
Ahmed Ibrahim
95212286d8 codex: fix developer instructions test config (#16976) 2026-04-06 21:35:40 -07:00
Ahmed Ibrahim
d15c7117f8 codex: preserve null developer instructions (#16964) 2026-04-06 21:18:52 -07:00
19 changed files with 121 additions and 1 deletions

View File

@@ -84,6 +84,7 @@ pub fn create_fake_rollout_with_source(
agent_role: None,
model_provider: model_provider.map(str::to_string),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
};
@@ -167,6 +168,7 @@ pub fn create_fake_rollout_with_text_elements(
agent_role: None,
model_provider: model_provider.map(str::to_string),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
};

View File

@@ -200,6 +200,16 @@ async fn thread_fork_honors_explicit_null_thread_instructions() -> Result<()> {
let codex_home = TempDir::new()?;
create_config_toml(codex_home.path(), &server.uri())?;
let config_path = codex_home.path().join("config.toml");
let mut config_toml = std::fs::read_to_string(&config_path)?;
let first_table_index = config_toml
.find("\n[")
.expect("test config must include a table header");
config_toml.insert_str(
first_table_index,
"\ndeveloper_instructions = \"Config developer instructions sentinel\"\n",
);
std::fs::write(config_path, config_toml)?;
let conversation_id = create_fake_rollout(
codex_home.path(),
@@ -323,6 +333,13 @@ async fn thread_fork_honors_explicit_null_thread_instructions() -> Result<()> {
"unexpected instructions field in payload: {payload:?}"
);
let developer_texts = request.message_input_texts("developer");
assert_eq!(
developer_texts
.iter()
.any(|text| { text.contains("Config developer instructions sentinel") }),
expect_instructions,
"unexpected config developer instruction presence: {developer_texts:?}"
);
assert!(
developer_texts.iter().all(|text| !text.is_empty()),
"did not expect empty developer instruction messages: {developer_texts:?}"

View File

@@ -377,6 +377,7 @@ stream_max_retries = 0
agent_role: None,
model_provider: Some("mock_provider".to_string()),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
};

View File

@@ -166,6 +166,16 @@ async fn turn_start_honors_explicit_null_thread_instructions() -> Result<()> {
let codex_home = TempDir::new()?;
create_config_toml(codex_home.path(), &server.uri(), "never", &BTreeMap::new())?;
let config_path = codex_home.path().join("config.toml");
let mut config_toml = std::fs::read_to_string(&config_path)?;
let first_table_index = config_toml
.find("\n[")
.expect("test config must include a table header");
config_toml.insert_str(
first_table_index,
"\ndeveloper_instructions = \"Config developer instructions sentinel\"\n",
);
std::fs::write(config_path, config_toml)?;
let mut mcp = McpProcess::new(codex_home.path()).await?;
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
@@ -240,6 +250,13 @@ async fn turn_start_honors_explicit_null_thread_instructions() -> Result<()> {
"unexpected instructions field in payload: {payload:?}"
);
let developer_texts = request.message_input_texts("developer");
assert_eq!(
developer_texts
.iter()
.any(|text| { text.contains("Config developer instructions sentinel") }),
expect_instructions,
"unexpected config developer instruction presence: {developer_texts:?}"
);
assert!(
developer_texts.iter().all(|text| !text.is_empty()),
"did not expect empty developer instruction messages: {developer_texts:?}"

View File

@@ -618,6 +618,14 @@ impl Codex {
dynamic_tools
};
let developer_instructions_override = config
.developer_instructions_override
.clone()
.or_else(|| conversation_history.get_developer_instructions());
let developer_instructions = developer_instructions_override
.clone()
.unwrap_or_else(|| config.developer_instructions.clone());
// TODO (aibrahim): Consolidate config.model and config.model_reasoning_effort into config.collaboration_mode
// to avoid extracting these fields separately and constructing CollaborationMode here.
let collaboration_mode = CollaborationMode {
@@ -633,7 +641,8 @@ impl Codex {
collaboration_mode,
model_reasoning_summary: config.model_reasoning_summary,
service_tier: config.service_tier,
developer_instructions: config.developer_instructions.clone(),
developer_instructions,
developer_instructions_override,
user_instructions,
personality: config.personality,
base_instructions,
@@ -1103,6 +1112,10 @@ pub(crate) struct SessionConfiguration {
/// Developer instructions that supplement the base instructions.
developer_instructions: Option<String>,
/// Explicit developer instructions override, preserving `null` as distinct
/// from a missing override.
developer_instructions_override: Option<Option<String>>,
/// Model instructions that are appended to the base instructions.
user_instructions: Option<String>,
@@ -1553,6 +1566,9 @@ impl Session {
.base_instructions
.clone()
.map(|text| BaseInstructions { text }),
session_configuration
.developer_instructions_override
.clone(),
session_configuration.dynamic_tools.clone(),
if session_configuration.persist_extended_history {
EventPersistenceMode::Extended

View File

@@ -1853,6 +1853,7 @@ async fn set_rate_limits_retains_previous_credits() {
collaboration_mode,
model_reasoning_summary: config.model_reasoning_summary,
developer_instructions: config.developer_instructions.clone(),
developer_instructions_override: config.developer_instructions_override.clone(),
user_instructions: config.user_instructions.clone(),
service_tier: None,
personality: config.personality,
@@ -1955,6 +1956,7 @@ async fn set_rate_limits_updates_plan_type_when_present() {
collaboration_mode,
model_reasoning_summary: config.model_reasoning_summary,
developer_instructions: config.developer_instructions.clone(),
developer_instructions_override: config.developer_instructions_override.clone(),
user_instructions: config.user_instructions.clone(),
service_tier: None,
personality: config.personality,
@@ -2227,6 +2229,7 @@ async fn attach_rollout_recorder(session: &Arc<Session>) -> PathBuf {
/*forked_from_id*/ None,
SessionSource::Exec,
Some(BaseInstructions::default()),
/*developer_instructions*/ None,
Vec::new(),
EventPersistenceMode::Limited,
),
@@ -2304,6 +2307,7 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati
collaboration_mode,
model_reasoning_summary: config.model_reasoning_summary,
developer_instructions: config.developer_instructions.clone(),
developer_instructions_override: config.developer_instructions_override.clone(),
user_instructions: config.user_instructions.clone(),
service_tier: None,
personality: config.personality,
@@ -2570,6 +2574,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() {
collaboration_mode,
model_reasoning_summary: config.model_reasoning_summary,
developer_instructions: config.developer_instructions.clone(),
developer_instructions_override: config.developer_instructions_override.clone(),
user_instructions: config.user_instructions.clone(),
service_tier: None,
personality: config.personality,
@@ -2673,6 +2678,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
collaboration_mode,
model_reasoning_summary: config.model_reasoning_summary,
developer_instructions: config.developer_instructions.clone(),
developer_instructions_override: config.developer_instructions_override.clone(),
user_instructions: config.user_instructions.clone(),
service_tier: None,
personality: config.personality,
@@ -3513,6 +3519,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
collaboration_mode,
model_reasoning_summary: config.model_reasoning_summary,
developer_instructions: config.developer_instructions.clone(),
developer_instructions_override: config.developer_instructions_override.clone(),
user_instructions: config.user_instructions.clone(),
service_tier: None,
personality: config.personality,
@@ -4269,6 +4276,7 @@ async fn record_context_updates_and_set_reference_context_item_persists_baseline
/*forked_from_id*/ None,
SessionSource::Exec,
Some(BaseInstructions::default()),
/*developer_instructions*/ None,
Vec::new(),
EventPersistenceMode::Limited,
),
@@ -4366,6 +4374,7 @@ async fn record_context_updates_and_set_reference_context_item_persists_full_rei
/*forked_from_id*/ None,
SessionSource::Exec,
Some(BaseInstructions::default()),
/*developer_instructions*/ None,
Vec::new(),
EventPersistenceMode::Limited,
),

View File

@@ -4506,6 +4506,7 @@ fn test_precedence_fixture_with_o3_profile() -> std::io::Result<()> {
experimental_realtime_ws_startup_context: None,
base_instructions: None,
developer_instructions: None,
developer_instructions_override: None,
guardian_developer_instructions: None,
include_permissions_instructions: true,
include_apps_instructions: true,
@@ -4651,6 +4652,7 @@ fn test_precedence_fixture_with_gpt3_profile() -> std::io::Result<()> {
experimental_realtime_ws_startup_context: None,
base_instructions: None,
developer_instructions: None,
developer_instructions_override: None,
guardian_developer_instructions: None,
include_permissions_instructions: true,
include_apps_instructions: true,
@@ -4794,6 +4796,7 @@ fn test_precedence_fixture_with_zdr_profile() -> std::io::Result<()> {
experimental_realtime_ws_startup_context: None,
base_instructions: None,
developer_instructions: None,
developer_instructions_override: None,
guardian_developer_instructions: None,
include_permissions_instructions: true,
include_apps_instructions: true,
@@ -4923,6 +4926,7 @@ fn test_precedence_fixture_with_gpt5_profile() -> std::io::Result<()> {
experimental_realtime_ws_startup_context: None,
base_instructions: None,
developer_instructions: None,
developer_instructions_override: None,
guardian_developer_instructions: None,
include_permissions_instructions: true,
include_apps_instructions: true,

View File

@@ -248,6 +248,10 @@ pub struct Config {
/// Developer instructions override injected as a separate message.
pub developer_instructions: Option<String>,
/// Explicit developer instructions override, preserving `null` as distinct
/// from a missing override.
pub developer_instructions_override: Option<Option<String>>,
/// Guardian-specific developer instructions override from requirements.toml.
pub guardian_developer_instructions: Option<String>,
@@ -1761,6 +1765,7 @@ impl Config {
let file_base_instructions =
Self::try_read_non_empty_file(model_instructions_path, "model instructions file")?;
let base_instructions = base_instructions.or_else(|| file_base_instructions.map(Some));
let developer_instructions_override = developer_instructions.clone();
let developer_instructions =
developer_instructions.unwrap_or_else(|| cfg.developer_instructions.clone());
let include_permissions_instructions = config_profile
@@ -1945,6 +1950,7 @@ impl Config {
base_instructions,
personality,
developer_instructions,
developer_instructions_override,
compact_prompt,
commit_attribution,
include_permissions_instructions,

View File

@@ -43,6 +43,7 @@ async fn write_session_with_user_event(codex_home: &Path) -> io::Result<()> {
agent_role: None,
model_provider: None,
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
},

View File

@@ -71,6 +71,7 @@ async fn write_rollout_with_user_event(dir: &Path, thread_id: ThreadId) -> io::R
agent_role: None,
model_provider: None,
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
},
@@ -116,6 +117,7 @@ async fn write_rollout_with_meta_only(dir: &Path, thread_id: ThreadId) -> io::Re
agent_role: None,
model_provider: None,
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
},

View File

@@ -174,6 +174,7 @@ async fn find_locates_rollout_file_written_by_recorder() -> std::io::Result<()>
/*forked_from_id*/ None,
SessionSource::Exec,
Some(BaseInstructions::default()),
/*developer_instructions*/ None,
Vec::new(),
EventPersistenceMode::Limited,
),

View File

@@ -146,6 +146,7 @@ async fn backfill_scans_existing_rollouts() -> Result<()> {
agent_role: None,
model_provider: None,
base_instructions: None,
developer_instructions: None,
dynamic_tools: Some(dynamic_tools_for_hook),
memory_mode: None,
},

View File

@@ -2351,6 +2351,26 @@ impl InitialHistory {
}
}
pub fn get_developer_instructions(&self) -> Option<Option<String>> {
match self {
InitialHistory::New => None,
InitialHistory::Resumed(resumed) => {
resumed.history.iter().find_map(|item| match item {
RolloutItem::SessionMeta(meta_line) => {
meta_line.meta.developer_instructions.clone()
}
_ => None,
})
}
InitialHistory::Forked(items) => items.iter().find_map(|item| match item {
RolloutItem::SessionMeta(meta_line) => {
meta_line.meta.developer_instructions.clone()
}
_ => None,
}),
}
}
pub fn get_dynamic_tools(&self) -> Option<Vec<DynamicToolSpec>> {
match self {
InitialHistory::New => None,
@@ -2549,6 +2569,13 @@ pub struct SessionMeta {
skip_serializing_if = "Option::is_none"
)]
pub base_instructions: Option<Option<BaseInstructions>>,
#[serde(
default,
deserialize_with = "deserialize_double_option",
serialize_with = "serialize_double_option",
skip_serializing_if = "Option::is_none"
)]
pub developer_instructions: Option<Option<String>>,
#[serde(skip_serializing_if = "Option::is_none")]
pub dynamic_tools: Option<Vec<DynamicToolSpec>>,
#[serde(skip_serializing_if = "Option::is_none")]
@@ -2570,6 +2597,7 @@ impl Default for SessionMeta {
agent_path: None,
model_provider: None,
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
}

View File

@@ -56,6 +56,7 @@ async fn extract_metadata_from_rollout_uses_session_meta() {
agent_role: None,
model_provider: Some("openai".to_string()),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
};
@@ -107,6 +108,7 @@ async fn extract_metadata_from_rollout_returns_latest_memory_mode() {
agent_role: None,
model_provider: Some("openai".to_string()),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
};
@@ -369,6 +371,7 @@ fn write_rollout_in_sessions_with_cwd(
agent_role: None,
model_provider: Some("test-provider".to_string()),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
};

View File

@@ -82,6 +82,7 @@ pub enum RolloutRecorderParams {
forked_from_id: Option<ThreadId>,
source: SessionSource,
base_instructions: Option<BaseInstructions>,
developer_instructions: Option<Option<String>>,
dynamic_tools: Vec<DynamicToolSpec>,
event_persistence_mode: EventPersistenceMode,
},
@@ -111,6 +112,7 @@ impl RolloutRecorderParams {
forked_from_id: Option<ThreadId>,
source: SessionSource,
base_instructions: Option<BaseInstructions>,
developer_instructions: Option<Option<String>>,
dynamic_tools: Vec<DynamicToolSpec>,
event_persistence_mode: EventPersistenceMode,
) -> Self {
@@ -119,6 +121,7 @@ impl RolloutRecorderParams {
forked_from_id,
source,
base_instructions,
developer_instructions,
dynamic_tools,
event_persistence_mode,
}
@@ -380,6 +383,7 @@ impl RolloutRecorder {
forked_from_id,
source,
base_instructions,
developer_instructions,
dynamic_tools,
event_persistence_mode,
} => {
@@ -409,6 +413,7 @@ impl RolloutRecorder {
source,
model_provider: Some(config.model_provider_id().to_string()),
base_instructions: Some(base_instructions),
developer_instructions,
dynamic_tools: if dynamic_tools.is_empty() {
None
} else {

View File

@@ -74,6 +74,7 @@ async fn recorder_materializes_only_after_explicit_persist() -> std::io::Result<
/*forked_from_id*/ None,
SessionSource::Exec,
Some(BaseInstructions::default()),
/*developer_instructions*/ None,
Vec::new(),
EventPersistenceMode::Limited,
),
@@ -167,6 +168,7 @@ async fn metadata_irrelevant_events_touch_state_db_updated_at() -> std::io::Resu
/*forked_from_id*/ None,
SessionSource::Cli,
Some(BaseInstructions::default()),
/*developer_instructions*/ None,
Vec::new(),
EventPersistenceMode::Limited,
),

View File

@@ -1140,6 +1140,7 @@ async fn test_updated_at_uses_file_mtime() -> Result<()> {
agent_role: None,
model_provider: Some("test-provider".into()),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
},

View File

@@ -257,6 +257,7 @@ mod tests {
agent_role: None,
model_provider: Some("openai".to_string()),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
},
@@ -384,6 +385,7 @@ mod tests {
agent_role: None,
model_provider: Some("openai".to_string()),
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
},

View File

@@ -1030,6 +1030,7 @@ mod tests {
agent_role: None,
model_provider: None,
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: Some("polluted".to_string()),
},
@@ -1088,6 +1089,7 @@ mod tests {
agent_role: None,
model_provider: None,
base_instructions: None,
developer_instructions: None,
dynamic_tools: None,
memory_mode: None,
},