Honor null thread instructions (#16964)

- Treat explicit null thread instructions as a blank-slate override
while preserving omitted-field fallback behavior.
- Preserve null through rollout resume/fork and keep explicit empty
strings distinct.
- Add app-server v2 start/fork coverage for the tri-state instruction
params.
This commit is contained in:
Ahmed Ibrahim
2026-04-07 04:10:19 +00:00
committed by GitHub
parent 4bb507d2c4
commit 24c598e8a9
39 changed files with 550 additions and 101 deletions
+10 -3
View File
@@ -399,7 +399,11 @@ impl ModelClient {
ApiCompactClient::new(transport, client_setup.api_provider, client_setup.api_auth)
.with_telemetry(Some(request_telemetry));
let instructions = prompt.base_instructions.text.clone();
let instructions = prompt
.base_instructions
.as_ref()
.map(|base_instructions| base_instructions.text.clone())
.unwrap_or_default();
let input = prompt.get_formatted_input();
let tools = create_tools_json_for_responses_api(&prompt.tools)?;
let reasoning = Self::build_reasoning(model_info, effort, summary);
@@ -755,7 +759,10 @@ impl ModelClientSession {
summary: ReasoningSummaryConfig,
service_tier: Option<ServiceTier>,
) -> Result<ResponsesApiRequest> {
let instructions = &prompt.base_instructions.text;
let instructions = prompt
.base_instructions
.as_ref()
.map(|base_instructions| base_instructions.text.clone());
let input = prompt.get_formatted_input();
let tools = create_tools_json_for_responses_api(&prompt.tools)?;
let default_reasoning_effort = model_info.default_reasoning_level;
@@ -794,7 +801,7 @@ impl ModelClientSession {
let prompt_cache_key = Some(self.client.state.conversation_id.to_string());
let request = ResponsesApiRequest {
model: model_info.slug.clone(),
instructions: instructions.clone(),
instructions,
input,
tools,
tool_choice: "auto".to_string(),
+15 -2
View File
@@ -23,7 +23,7 @@ pub const REVIEW_EXIT_INTERRUPTED_TMPL: &str =
include_str!("../templates/review/exit_interrupted.xml");
/// API request payload for a single model turn
#[derive(Default, Debug, Clone)]
#[derive(Debug, Clone)]
pub struct Prompt {
/// Conversation context input items.
pub input: Vec<ResponseItem>,
@@ -35,7 +35,7 @@ pub struct Prompt {
/// Whether parallel tool calls are permitted for this prompt.
pub(crate) parallel_tool_calls: bool,
pub base_instructions: BaseInstructions,
pub base_instructions: Option<BaseInstructions>,
/// Optionally specify the personality of the model.
pub personality: Option<Personality>,
@@ -44,6 +44,19 @@ pub struct Prompt {
pub output_schema: Option<Value>,
}
impl Default for Prompt {
fn default() -> Self {
Self {
input: Vec::new(),
tools: Vec::new(),
parallel_tool_calls: false,
base_instructions: Some(BaseInstructions::default()),
personality: None,
output_schema: None,
}
}
}
impl Prompt {
pub(crate) fn get_formatted_input(&self) -> Vec<ResponseItem> {
let mut input = self.input.clone();
+4 -4
View File
@@ -14,7 +14,7 @@ fn serializes_text_verbosity_when_set() {
let tools: Vec<serde_json::Value> = vec![];
let req = ResponsesApiRequest {
model: "gpt-5.1".to_string(),
instructions: "i".to_string(),
instructions: Some("i".to_string()),
input,
tools,
tool_choice: "auto".to_string(),
@@ -57,7 +57,7 @@ fn serializes_text_schema_with_strict_format() {
let req = ResponsesApiRequest {
model: "gpt-5.1".to_string(),
instructions: "i".to_string(),
instructions: Some("i".to_string()),
input,
tools,
tool_choice: "auto".to_string(),
@@ -94,7 +94,7 @@ fn omits_text_when_not_set() {
let tools: Vec<serde_json::Value> = vec![];
let req = ResponsesApiRequest {
model: "gpt-5.1".to_string(),
instructions: "i".to_string(),
instructions: Some("i".to_string()),
input,
tools,
tool_choice: "auto".to_string(),
@@ -116,7 +116,7 @@ fn omits_text_when_not_set() {
fn serializes_flex_service_tier_when_set() {
let req = ResponsesApiRequest {
model: "gpt-5.1".to_string(),
instructions: "i".to_string(),
instructions: Some("i".to_string()),
input: vec![],
tools: vec![],
tool_choice: "auto".to_string(),
+36 -18
View File
@@ -581,11 +581,15 @@ impl Codex {
let model_info = models_manager
.get_model_info(model.as_str(), &config.to_models_manager_config())
.await;
let base_instructions = config
.base_instructions
.clone()
.or_else(|| conversation_history.get_base_instructions().map(|s| s.text))
.unwrap_or_else(|| model_info.get_model_instructions(config.personality));
let base_instructions = match config.base_instructions.clone() {
Some(base_instructions) => base_instructions,
None => conversation_history
.get_base_instructions()
.map(|base_instructions| {
base_instructions.map(|base_instructions| base_instructions.text)
})
.unwrap_or_else(|| Some(model_info.get_model_instructions(config.personality))),
};
// Respect thread-start tools. When missing (resumed/forked threads), read from the db
// first, then fall back to rollout-file tools.
@@ -1106,7 +1110,7 @@ pub(crate) struct SessionConfiguration {
personality: Option<Personality>,
/// Base instructions for the session.
base_instructions: String,
base_instructions: Option<String>,
/// Compact prompt override.
compact_prompt: Option<String>,
@@ -1545,9 +1549,10 @@ impl Session {
conversation_id,
forked_from_id,
session_source,
BaseInstructions {
text: session_configuration.base_instructions.clone(),
},
session_configuration
.base_instructions
.clone()
.map(|text| BaseInstructions { text }),
session_configuration.dynamic_tools.clone(),
if session_configuration.persist_extended_history {
EventPersistenceMode::Extended
@@ -2109,8 +2114,9 @@ impl Session {
));
}
}
sess.schedule_startup_prewarm(session_configuration.base_instructions.clone())
.await;
if let Some(base_instructions) = session_configuration.base_instructions.clone() {
sess.schedule_startup_prewarm(base_instructions).await;
}
let session_start_source = match &initial_history {
InitialHistory::Resumed(_) => codex_hooks::SessionStartSource::Resume,
InitialHistory::New | InitialHistory::Forked(_) => {
@@ -2212,11 +2218,13 @@ impl Session {
state.history.estimate_token_count(turn_context)
}
pub(crate) async fn get_base_instructions(&self) -> BaseInstructions {
pub(crate) async fn get_base_instructions(&self) -> Option<BaseInstructions> {
let state = self.state.lock().await;
BaseInstructions {
text: state.session_configuration.base_instructions.clone(),
}
state
.session_configuration
.base_instructions
.clone()
.map(|text| BaseInstructions { text })
}
// Merges connector IDs into the session-level explicit connector selection.
@@ -3620,7 +3628,11 @@ impl Session {
state.reference_context_item(),
state.previous_turn_settings(),
state.session_configuration.collaboration_mode.clone(),
state.session_configuration.base_instructions.clone(),
state
.session_configuration
.base_instructions
.clone()
.unwrap_or_default(),
state.session_configuration.session_source.clone(),
)
};
@@ -3861,7 +3873,13 @@ impl Session {
pub(crate) async fn recompute_token_usage(&self, turn_context: &TurnContext) {
let history = self.clone_history().await;
let base_instructions = self.get_base_instructions().await;
let empty_base_instructions = BaseInstructions {
text: String::new(),
};
let base_instructions = self
.get_base_instructions()
.await
.unwrap_or(empty_base_instructions);
let Some(estimated_total_tokens) =
history.estimate_token_count_with_base_instructions(&base_instructions)
else {
@@ -6555,7 +6573,7 @@ pub(crate) fn build_prompt(
input: Vec<ResponseItem>,
router: &ToolRouter,
turn_context: &TurnContext,
base_instructions: BaseInstructions,
base_instructions: Option<BaseInstructions>,
) -> Prompt {
let deferred_dynamic_tools = turn_context
.dynamic_tools
+16 -12
View File
@@ -591,11 +591,15 @@ async fn get_base_instructions_no_user_content() {
{
let mut state = session.state.lock().await;
state.session_configuration.base_instructions = model_info.base_instructions.clone();
state.session_configuration.base_instructions =
Some(model_info.base_instructions.clone());
}
let base_instructions = session.get_base_instructions().await;
assert_eq!(base_instructions.text, model_info.base_instructions);
assert_eq!(
base_instructions.expect("base instructions").text,
model_info.base_instructions
);
}
}
@@ -1091,7 +1095,7 @@ async fn recompute_token_usage_uses_session_base_instructions() {
let override_instructions = "SESSION_OVERRIDE_INSTRUCTIONS_ONLY".repeat(120);
{
let mut state = session.state.lock().await;
state.session_configuration.base_instructions = override_instructions.clone();
state.session_configuration.base_instructions = Some(override_instructions.clone());
}
let item = user_message("hello");
@@ -1855,7 +1859,7 @@ async fn set_rate_limits_retains_previous_credits() {
base_instructions: config
.base_instructions
.clone()
.unwrap_or_else(|| model_info.get_model_instructions(config.personality)),
.unwrap_or_else(|| Some(model_info.get_model_instructions(config.personality))),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.permissions.approval_policy.clone(),
approvals_reviewer: config.approvals_reviewer,
@@ -1957,7 +1961,7 @@ async fn set_rate_limits_updates_plan_type_when_present() {
base_instructions: config
.base_instructions
.clone()
.unwrap_or_else(|| model_info.get_model_instructions(config.personality)),
.unwrap_or_else(|| Some(model_info.get_model_instructions(config.personality))),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.permissions.approval_policy.clone(),
approvals_reviewer: config.approvals_reviewer,
@@ -2222,7 +2226,7 @@ async fn attach_rollout_recorder(session: &Arc<Session>) -> PathBuf {
ThreadId::default(),
/*forked_from_id*/ None,
SessionSource::Exec,
BaseInstructions::default(),
Some(BaseInstructions::default()),
Vec::new(),
EventPersistenceMode::Limited,
),
@@ -2306,7 +2310,7 @@ pub(crate) async fn make_session_configuration_for_tests() -> SessionConfigurati
base_instructions: config
.base_instructions
.clone()
.unwrap_or_else(|| model_info.get_model_instructions(config.personality)),
.unwrap_or_else(|| Some(model_info.get_model_instructions(config.personality))),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.permissions.approval_policy.clone(),
approvals_reviewer: config.approvals_reviewer,
@@ -2572,7 +2576,7 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() {
base_instructions: config
.base_instructions
.clone()
.unwrap_or_else(|| model_info.get_model_instructions(config.personality)),
.unwrap_or_else(|| Some(model_info.get_model_instructions(config.personality))),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.permissions.approval_policy.clone(),
approvals_reviewer: config.approvals_reviewer,
@@ -2675,7 +2679,7 @@ pub(crate) async fn make_session_and_context() -> (Session, TurnContext) {
base_instructions: config
.base_instructions
.clone()
.unwrap_or_else(|| model_info.get_model_instructions(config.personality)),
.unwrap_or_else(|| Some(model_info.get_model_instructions(config.personality))),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.permissions.approval_policy.clone(),
approvals_reviewer: config.approvals_reviewer,
@@ -3515,7 +3519,7 @@ pub(crate) async fn make_session_and_context_with_dynamic_tools_and_rx(
base_instructions: config
.base_instructions
.clone()
.unwrap_or_else(|| model_info.get_model_instructions(config.personality)),
.unwrap_or_else(|| Some(model_info.get_model_instructions(config.personality))),
compact_prompt: config.compact_prompt.clone(),
approval_policy: config.permissions.approval_policy.clone(),
approvals_reviewer: config.approvals_reviewer,
@@ -4264,7 +4268,7 @@ async fn record_context_updates_and_set_reference_context_item_persists_baseline
ThreadId::default(),
/*forked_from_id*/ None,
SessionSource::Exec,
BaseInstructions::default(),
Some(BaseInstructions::default()),
Vec::new(),
EventPersistenceMode::Limited,
),
@@ -4361,7 +4365,7 @@ async fn record_context_updates_and_set_reference_context_item_persists_full_rei
ThreadId::default(),
/*forked_from_id*/ None,
SessionSource::Exec,
BaseInstructions::default(),
Some(BaseInstructions::default()),
Vec::new(),
EventPersistenceMode::Limited,
),
+13 -2
View File
@@ -76,10 +76,16 @@ async fn run_remote_compact_task_inner_impl(
.await;
let mut history = sess.clone_history().await;
let base_instructions = sess.get_base_instructions().await;
let token_count_base_instructions =
base_instructions
.clone()
.unwrap_or_else(|| BaseInstructions {
text: String::new(),
});
let deleted_items = trim_function_call_history_to_fit_context_window(
&mut history,
turn_context.as_ref(),
&base_instructions,
&token_count_base_instructions,
);
if deleted_items > 0 {
info!(
@@ -127,8 +133,13 @@ async fn run_remote_compact_task_inner_impl(
)
.or_else(|err| async {
let total_usage_breakdown = sess.get_total_token_usage_breakdown().await;
let base_instruction_text = prompt
.base_instructions
.as_ref()
.map(|base_instructions| base_instructions.text.as_str())
.unwrap_or("");
let compact_request_log_data =
build_compact_request_log_data(&prompt.input, &prompt.base_instructions.text);
build_compact_request_log_data(&prompt.input, base_instruction_text);
log_remote_compact_failure(
turn_context,
&compact_request_log_data,
+7 -6
View File
@@ -243,7 +243,7 @@ pub struct Config {
pub user_instructions: Option<String>,
/// Base instructions override.
pub base_instructions: Option<String>,
pub base_instructions: Option<Option<String>>,
/// Developer instructions override injected as a separate message.
pub developer_instructions: Option<String>,
@@ -687,7 +687,7 @@ impl Config {
model_context_window: self.model_context_window,
model_auto_compact_token_limit: self.model_auto_compact_token_limit,
tool_output_token_limit: self.tool_output_token_limit,
base_instructions: self.base_instructions.clone(),
base_instructions: self.base_instructions.clone().flatten(),
personality_enabled: self.features.enabled(Feature::Personality),
model_supports_reasoning_summaries: self.model_supports_reasoning_summaries,
model_catalog: self.model_catalog.clone(),
@@ -1200,8 +1200,8 @@ pub struct ConfigOverrides {
pub js_repl_node_path: Option<PathBuf>,
pub js_repl_node_module_dirs: Option<Vec<PathBuf>>,
pub zsh_path: Option<PathBuf>,
pub base_instructions: Option<String>,
pub developer_instructions: Option<String>,
pub base_instructions: Option<Option<String>>,
pub developer_instructions: Option<Option<String>>,
pub personality: Option<Personality>,
pub compact_prompt: Option<String>,
pub include_apply_patch_tool: Option<bool>,
@@ -1760,8 +1760,9 @@ impl Config {
.or(cfg.model_instructions_file.as_ref());
let file_base_instructions =
Self::try_read_non_empty_file(model_instructions_path, "model instructions file")?;
let base_instructions = base_instructions.or(file_base_instructions);
let developer_instructions = developer_instructions.or(cfg.developer_instructions);
let base_instructions = base_instructions.or_else(|| file_base_instructions.map(Some));
let developer_instructions =
developer_instructions.unwrap_or_else(|| cfg.developer_instructions.clone());
let include_permissions_instructions = config_profile
.include_permissions_instructions
.or(cfg.include_permissions_instructions)
+2 -2
View File
@@ -905,7 +905,7 @@ model_instructions_file = "child.txt"
.await?;
assert_eq!(
config.base_instructions.as_deref(),
config.base_instructions.as_ref().and_then(Option::as_deref),
Some("child instructions")
);
@@ -941,7 +941,7 @@ async fn cli_override_model_instructions_file_sets_base_instructions() -> std::i
.await?;
assert_eq!(
config.base_instructions.as_deref(),
config.base_instructions.as_ref().and_then(Option::as_deref),
Some("cli override instructions")
);
+1 -1
View File
@@ -137,7 +137,7 @@ impl GuardianReviewSessionReuseKey {
model_reasoning_summary: spawn_config.model_reasoning_summary,
permissions: spawn_config.permissions.clone(),
developer_instructions: spawn_config.developer_instructions.clone(),
base_instructions: spawn_config.base_instructions.clone(),
base_instructions: spawn_config.base_instructions.clone().flatten(),
user_instructions: spawn_config.user_instructions.clone(),
compact_prompt: spawn_config.compact_prompt.clone(),
cwd: spawn_config.cwd.to_path_buf(),
+2 -2
View File
@@ -336,9 +336,9 @@ mod job {
}],
tools: Vec::new(),
parallel_tool_calls: false,
base_instructions: BaseInstructions {
base_instructions: Some(BaseInstructions {
text: phase_one::PROMPT.to_string(),
},
}),
personality: None,
output_schema: Some(output_schema()),
};
+2 -2
View File
@@ -217,9 +217,9 @@ async fn schedule_startup_prewarm_inner(
Vec::new(),
startup_router.as_ref(),
startup_turn_context.as_ref(),
BaseInstructions {
Some(BaseInstructions {
text: base_instructions,
},
}),
);
let startup_turn_metadata_header = startup_turn_context
.turn_metadata_state
+1 -1
View File
@@ -112,7 +112,7 @@ async fn start_review_conversation(
let _ = sub_agent_config.features.disable(Feature::Collab);
// Set explicit review rubric for the sub-agent
sub_agent_config.base_instructions = Some(crate::REVIEW_PROMPT.to_string());
sub_agent_config.base_instructions = Some(Some(crate::REVIEW_PROMPT.to_string()));
sub_agent_config.permissions.approval_policy = Constrained::allow_only(AskForApproval::Never);
let model = config
@@ -535,7 +535,7 @@ async fn build_runner_options(
let max_concurrency =
normalize_concurrency(requested_concurrency, turn.config.agent_max_threads);
let base_instructions = session.get_base_instructions().await;
let spawn_config = build_agent_spawn_config(&base_instructions, turn.as_ref())?;
let spawn_config = build_agent_spawn_config(base_instructions.as_ref(), turn.as_ref())?;
Ok(JobRunnerOptions {
max_concurrency,
spawn_config,
@@ -59,8 +59,10 @@ impl ToolHandler for Handler {
.into(),
)
.await;
let mut config =
build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?;
let mut config = build_agent_spawn_config(
session.get_base_instructions().await.as_ref(),
turn.as_ref(),
)?;
apply_requested_spawn_agent_model_overrides(
&session,
turn.as_ref(),
@@ -201,11 +201,12 @@ pub(crate) fn parse_collab_input(
/// skipping this helper and cloning stale config state directly can send the child agent out with
/// the wrong provider or runtime policy.
pub(crate) fn build_agent_spawn_config(
base_instructions: &BaseInstructions,
base_instructions: Option<&BaseInstructions>,
turn: &TurnContext,
) -> Result<Config, FunctionCallError> {
let mut config = build_agent_shared_config(turn)?;
config.base_instructions = Some(base_instructions.text.clone());
config.base_instructions =
Some(base_instructions.map(|base_instructions| base_instructions.text.clone()));
Ok(config)
}
@@ -3209,9 +3209,9 @@ async fn build_agent_spawn_config_uses_turn_context_values() {
.set(AskForApproval::OnRequest)
.expect("approval policy set");
let config = build_agent_spawn_config(&base_instructions, &turn).expect("spawn config");
let config = build_agent_spawn_config(Some(&base_instructions), &turn).expect("spawn config");
let mut expected = (*turn.config).clone();
expected.base_instructions = Some(base_instructions.text);
expected.base_instructions = Some(Some(base_instructions.text));
expected.model = Some(turn.model_info.slug.clone());
expected.model_provider = turn.provider.clone();
expected.model_reasoning_effort = turn.reasoning_effort;
@@ -3247,7 +3247,7 @@ async fn build_agent_spawn_config_preserves_base_user_instructions() {
text: "base".to_string(),
};
let config = build_agent_spawn_config(&base_instructions, &turn).expect("spawn config");
let config = build_agent_spawn_config(Some(&base_instructions), &turn).expect("spawn config");
assert_eq!(config.user_instructions, base_config.user_instructions);
}
@@ -3256,7 +3256,7 @@ async fn build_agent_spawn_config_preserves_base_user_instructions() {
async fn build_agent_resume_config_clears_base_instructions() {
let (_session, mut turn) = make_session_and_context().await;
let mut base_config = (*turn.config).clone();
base_config.base_instructions = Some("caller-base".to_string());
base_config.base_instructions = Some(Some("caller-base".to_string()));
turn.config = Arc::new(base_config);
turn.approval_policy
.set(AskForApproval::OnRequest)
@@ -68,8 +68,10 @@ impl ToolHandler for Handler {
.into(),
)
.await;
let mut config =
build_agent_spawn_config(&session.get_base_instructions().await, turn.as_ref())?;
let mut config = build_agent_spawn_config(
session.get_base_instructions().await.as_ref(),
turn.as_ref(),
)?;
apply_requested_spawn_agent_model_overrides(
&session,
turn.as_ref(),