diff --git a/codex-rs/core/src/agent/control/spawn.rs b/codex-rs/core/src/agent/control/spawn.rs index 70cf8b7ef0..cc68bfdd3b 100644 --- a/codex-rs/core/src/agent/control/spawn.rs +++ b/codex-rs/core/src/agent/control/spawn.rs @@ -18,6 +18,7 @@ use crate::context::CurrentTimeReminder; use crate::context::CurrentTimeUnavailable; use crate::context::DeveloperInstructions; use crate::context::ManagedDeveloperInstructions; +use crate::context::ModelInstructions; use crate::context::MultiAgentModeInstructions; use crate::context::MultiAgentRoleInstructions; use crate::context::MultiAgentUsageHint; @@ -133,6 +134,10 @@ fn retain_forked_developer_message(item: &mut ResponseItem, usage_hint_texts: &[ return false; }; content.retain(|content_item| { + // Leave base instructions for the later context-baseline retention check. + if content_item.kind().as_str() == BaseInstructionsFragment::KIND { + return true; + } // Persisted role hints can predate the current bundled wording and lack markers. if matches!( content_item.kind().as_str(), @@ -1181,6 +1186,7 @@ impl LocalAgentControl { return true; }; if ManagedDeveloperInstructions::matches_text(text) + || ModelInstructions::matches_text(text) || PersistentModeState::matches_text(text) { // If the child will rebuild its initial context, drop the inherited diff --git a/codex-rs/core/src/agent/control_tests.rs b/codex-rs/core/src/agent/control_tests.rs index 6d97eceb2e..3f5ac6590b 100644 --- a/codex-rs/core/src/agent/control_tests.rs +++ b/codex-rs/core/src/agent/control_tests.rs @@ -21,8 +21,10 @@ use crate::agent_communication::AgentCommunicationKind; use crate::config::AgentRoleConfig; use crate::config::Config; use crate::config::ConfigBuilder; +use crate::context::BaseInstructionsFragment; use crate::context::ContextualUserFragment; use crate::context::ManagedDeveloperInstructions; +use crate::context::ModelInstructions; use crate::context::MultiAgentRoleInstructions; use crate::context::SubagentNotification; use crate::context::world_state::IncrementalToolsHint; @@ -47,6 +49,7 @@ use codex_history::ResumedHistory; use codex_history::RolloutItem; use codex_login::AuthManager; use codex_login::CodexAuth; +use codex_models_manager::model_info::model_info_from_slug; use codex_protocol::AgentPath; use codex_protocol::ResponseItemId; use codex_protocol::capabilities::CapabilityRootLocation; @@ -74,6 +77,7 @@ use codex_protocol::models::InternalChatMessageMetadataPassthrough; use codex_protocol::models::MessagePhase; use codex_protocol::models::PermissionProfile; use codex_protocol::models::ResponseItem; +use codex_protocol::openai_models::ModelsResponse; use codex_protocol::openai_models::ReasoningEffort; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::EnvironmentConfigState; @@ -2815,6 +2819,11 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { let managed_fragment = "\nParent developer instructions.\n"; let persistent_fragment = "\nParent developer instructions.\n"; + let replacement_fragment = ModelInstructions { + model: None, + instructions: Some("Updated model instructions.".to_string()), + } + .render(); let harness = AgentControlHarness::new().await; let mut parent_config = harness.config.clone(); let _ = parent_config.features.enable(Feature::MultiAgentV2); @@ -2933,6 +2942,9 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { ContentItem::InputText { text: persistent_fragment.to_string(), }, + ContentItem::InputText { + text: replacement_fragment.clone(), + }, ], phase: None, internal_chat_message_metadata_passthrough: Some( @@ -2943,6 +2955,7 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { ContentItemKind("generic.developer_policy".to_string()), ContentItemKind("managed_config.developer_instructions".to_string()), ContentItemKind("persistent_mode.instructions".to_string()), + ContentItemKind("model.instructions".to_string()), ]), ..Default::default() }, @@ -3084,6 +3097,9 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { ContentItem::InputText { text: persistent_fragment.to_string(), }, + ContentItem::InputText { + text: replacement_fragment, + }, ], phase: None, internal_chat_message_metadata_passthrough: Some( @@ -3093,6 +3109,7 @@ async fn spawn_agent_can_fork_parent_thread_history_with_sanitized_items() { ContentItemKind("generic.developer_policy".to_string()), ContentItemKind("managed_config.developer_instructions".to_string()), ContentItemKind("persistent_mode.instructions".to_string()), + ContentItemKind("model.instructions".to_string()), ]), ..Default::default() }, @@ -3689,9 +3706,16 @@ async fn spawn_agent_full_fork_legacy_compaction_rebuilds_child_instructions_onc Some("Parent developer instructions."), ), ] { - let harness = AgentControlHarness::new().await; + let mut harness = AgentControlHarness::new().await; let mut parent_config = harness.config.clone(); let _ = parent_config.features.enable(Feature::MultiAgentV2); + let _ = parent_config.features.enable(Feature::IncrementalTools); + let mut model = model_info_from_slug("gpt-5.5"); + model.use_responses_lite = true; + parent_config.model_catalog = Some(ModelsResponse { + models: vec![model], + }); + parent_config.base_instructions = Some("Current model instructions.".to_string()); parent_config.developer_instructions = parent_developer_instructions.map(str::to_string); let mut requirements = parent_config.config_layer_stack.requirements().clone(); requirements.additional_developer_instructions = Some(codex_config::Sourced::new( @@ -3710,6 +3734,28 @@ async fn spawn_agent_full_fork_legacy_compaction_rebuilds_child_instructions_onc requirements_toml, ) .expect("managed requirements stack"); + let auth_manager = AuthManager::from_auth_for_testing(CodexAuth::from_api_key("dummy")); + harness.manager = ThreadManager::new( + &parent_config, + auth_manager.clone(), + crate::thread_manager::build_models_manager(&parent_config, auth_manager), + crate::CodexAppsToolsCache::default(), + SessionSource::Exec, + Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()), + empty_extension_registry(), + Arc::new(crate::test_support::EmptyUserInstructionsProvider), + /*analytics_events_client*/ None, + crate::thread_manager::passthrough_image_store(), + crate::thread_manager::thread_store_from_config( + &parent_config, + harness.state_db.clone(), + ), + crate::thread_manager::local_agent_graph_store_from_state_db(harness.state_db.as_ref()), + uuid::Uuid::new_v4().to_string(), + /*attestation_provider*/ None, + /*external_time_provider*/ None, + ); + harness.control = harness.manager.agent_control(); let mut child_config = parent_config.clone(); child_config.multi_agent_v2.preserve_fork_prefix = preserve_fork_prefix; child_config.developer_instructions = Some("Child developer instructions.".to_string()); @@ -3800,6 +3846,15 @@ async fn spawn_agent_full_fork_legacy_compaction_rebuilds_child_instructions_onc phase: None, internal_chat_message_metadata_passthrough: None, })); + rollout_items.extend([ + rollout_response_item(ContextualUserFragment::into(BaseInstructionsFragment( + "Original model instructions.".to_string(), + ))), + rollout_response_item(ContextualUserFragment::into(ModelInstructions { + model: None, + instructions: Some("Updated model instructions.".to_string()), + })), + ]); rollout_items.push(rollout_response_item(ContextualUserFragment::into( IncrementalToolsHint( codex_prompts::ResolvedIncrementalToolMessages::default().tool_update_hint, @@ -3906,7 +3961,11 @@ async fn spawn_agent_full_fork_legacy_compaction_rebuilds_child_instructions_onc let history = child_thread.session.clone_history().await; let mut instruction_count = 0; let mut managed_instructions = Vec::new(); + let mut model_instructions = Vec::new(); for item in history.raw_items() { + if let Some(base) = BaseInstructionsFragment::text_from_item(item) { + model_instructions.push(base); + } let ResponseItem::Message { role, content, .. } = item else { continue; }; @@ -3916,6 +3975,9 @@ async fn spawn_agent_full_fork_legacy_compaction_rebuilds_child_instructions_onc for content_item in content { if let ContentItem::InputText { text } = content_item { instruction_count += usize::from(text == "Child developer instructions."); + if ModelInstructions::matches_text(text) { + model_instructions.push(text.as_str()); + } if ManagedDeveloperInstructions::matches_text(text) { managed_instructions.push(text.as_str()); } @@ -3923,9 +3985,13 @@ async fn spawn_agent_full_fork_legacy_compaction_rebuilds_child_instructions_onc } } assert_eq!( - (instruction_count, managed_instructions), - (1, vec![current_managed_fragment.as_str()]), - "{case}: canonical context reconstruction must keep only the current child and managed developer instructions" + (instruction_count, managed_instructions, model_instructions), + ( + 1, + vec![current_managed_fragment.as_str()], + vec!["Current model instructions."], + ), + "{case}: canonical context reconstruction must keep only the current child, managed, and model instructions" ); let _ = harness diff --git a/codex-rs/core/src/compact.rs b/codex-rs/core/src/compact.rs index abd7dfd570..03c60ee1cd 100644 --- a/codex-rs/core/src/compact.rs +++ b/codex-rs/core/src/compact.rs @@ -280,6 +280,16 @@ async fn run_compact_task_inner_impl( &[initial_input_for_turn.into()], turn_context.model_info().truncation_policy.into(), ); + let mut fallback_instructions = sess.get_prompt_base_instructions().await; + if let Some(text) = history + .raw_items() + .find_map(BaseInstructionsFragment::text_from_item) + { + fallback_instructions.text = text.to_owned(); + } else if history.has_tool_declarations() { + // An incremental window has no recorded base item when its instructions are empty. + fallback_instructions.text.clear(); + } let max_retries = turn_context.provider.info().stream_max_retries(); let mut retries = 0; @@ -297,14 +307,14 @@ async fn run_compact_task_inner_impl( let turn_input_len = turn_input.len(); let base_instructions = if turn_input .iter() - .any(BaseInstructionsFragment::matches_item) + .any(|item| BaseInstructionsFragment::text_from_item(item).is_some()) { BaseInstructions { text: String::new(), provenance: None, } } else { - sess.get_prompt_base_instructions().await + fallback_instructions.clone() }; let prompt = Prompt { input: turn_input, diff --git a/codex-rs/core/src/compact_tests.rs b/codex-rs/core/src/compact_tests.rs index 02be9c3499..cc80b0479b 100644 --- a/codex-rs/core/src/compact_tests.rs +++ b/codex-rs/core/src/compact_tests.rs @@ -27,7 +27,7 @@ use test_case::test_case; #[test_case(true; "metadata enabled")] #[test_case(false; "metadata disabled after capture")] #[tokio::test] -async fn local_compaction_respects_tool_metadata_state( +async fn local_compaction_retries_preserve_instructions_and_tool_metadata( metadata_enabled: bool, ) -> anyhow::Result<()> { skip_if_no_network!(Ok(())); @@ -48,7 +48,13 @@ async fn local_compaction_respects_tool_metadata_state( ) .await; - let mut items = vec![user_message("Update the plan")]; + // Retries trim the original prefix and early user messages. + let mut items = vec![ + ContextualUserFragment::into(BaseInstructionsFragment("Window A".to_string())), + user_message("First turn"), + user_message("Rebuilt context"), + user_message("Update the plan"), + ]; for index in 0..300 { let call_id = format!("direct-{index}"); let arguments = json!({"plan": [{"step": "x".repeat(7 * 1024), "status": "completed"}]}); @@ -152,12 +158,17 @@ async fn local_compaction_respects_tool_metadata_state( let _ = session.refresh_runtime_config(current_config, config).await; } - let mock = responses::mount_sse_once( + let failed = responses::sse_failed("failed", "context_length_exceeded", "Context too long"); + let mock = responses::mount_sse_sequence( &server, - responses::sse(vec![ - responses::ev_assistant_message("summary", "The prior calls finished."), - responses::ev_completed("compact-response"), - ]), + vec![ + failed.clone(), + failed, + responses::sse(vec![ + responses::ev_assistant_message("summary", "The prior calls finished."), + responses::ev_completed("compact-response"), + ]), + ], ) .await; let step = session @@ -180,16 +191,26 @@ async fn local_compaction_respects_tool_metadata_state( ) .await?; - let request = mock.single_request(); - assert!(request.inputs_of_type("compaction_trigger").is_empty()); + let requests = mock.requests(); assert_eq!( - request.custom_tool_call_output("exec"), - serde_json::to_value(expected_code_mode_output)?, + requests + .iter() + .map(|request| request.message_input_texts("developer").into_iter().next()) + .collect::>(), + [ + Some("Window A".to_string()), + Some("Window A".to_string()), + Some("Window A".to_string()) + ], ); + for request in &requests { + assert!(request.inputs_of_type("compaction_trigger").is_empty()); + assert_eq!( + request.custom_tool_call_output("exec"), + serde_json::to_value(&expected_code_mode_output)?, + ); + } for mut output in outputs { - let call_id = output["call_id"].as_str().expect("source call id"); - let compact_output = request.function_call_output(call_id); - assert_eq!(compact_output["output"], json!("Plan updated")); if !metadata_enabled { let metadata = output["internal_chat_message_metadata_passthrough"] .as_object_mut() @@ -197,10 +218,15 @@ async fn local_compaction_respects_tool_metadata_state( metadata.remove("executed_tool_calls"); metadata.remove("tool_calls_complete"); } - assert_eq!( - compact_output["internal_chat_message_metadata_passthrough"], - output["internal_chat_message_metadata_passthrough"] - ); + let call_id = output["call_id"].as_str().expect("source call id"); + for request in &requests { + let compact_output = request.function_call_output(call_id); + assert_eq!(compact_output["output"], json!("Plan updated")); + assert_eq!( + compact_output["internal_chat_message_metadata_passthrough"], + output["internal_chat_message_metadata_passthrough"] + ); + } } let compacted_history = session.clone_history().await; let expected_summary = format!("{SUMMARY_PREFIX}\nThe prior calls finished."); diff --git a/codex-rs/core/src/context/base_instructions.rs b/codex-rs/core/src/context/base_instructions.rs index 6ef7d583c6..d81895a243 100644 --- a/codex-rs/core/src/context/base_instructions.rs +++ b/codex-rs/core/src/context/base_instructions.rs @@ -1,4 +1,7 @@ +//! Initial base instructions, recorded in history for incremental-tools windows. + use super::ContextualUserFragment; +use codex_protocol::models::ContentItem; use codex_protocol::models::ContentItemKind; use codex_protocol::models::ResponseItem; @@ -8,18 +11,29 @@ pub(crate) struct BaseInstructionsFragment(pub(crate) String); impl BaseInstructionsFragment { pub(crate) const KIND: &str = "model.base_instructions"; - pub(crate) fn matches_item(item: &ResponseItem) -> bool { + /// Reads only a dedicated, harness-classified base item, including an explicitly empty base. + pub(crate) fn text_from_item(item: &ResponseItem) -> Option<&str> { let ResponseItem::Message { + role, + content, internal_chat_message_metadata_passthrough: Some(metadata), .. } = item else { - return false; + return None; }; - metadata - .content_item_kinds - .as_ref() - .is_some_and(|kinds| kinds.iter().any(|kind| kind.as_str() == Self::KIND)) + match ( + role.as_str(), + content.as_slice(), + metadata.content_item_kinds.as_deref(), + ) { + ("developer", [ContentItem::InputText { text }], Some([kind])) + if kind.as_str() == Self::KIND => + { + Some(text) + } + _ => None, + } } } diff --git a/codex-rs/core/src/context/mod.rs b/codex-rs/core/src/context/mod.rs index 4f2d3f9e2b..a6a4f600e5 100644 --- a/codex-rs/core/src/context/mod.rs +++ b/codex-rs/core/src/context/mod.rs @@ -35,6 +35,7 @@ mod legacy_apply_patch_exec_command_warning; mod legacy_model_mismatch_warning; mod legacy_unified_exec_process_limit_warning; mod memory; +mod model_instructions; mod model_switch_instructions; mod multi_agent_mode_instructions; mod multi_agent_usage_hint; @@ -108,6 +109,7 @@ pub(crate) use legacy_apply_patch_exec_command_warning::LegacyApplyPatchExecComm pub(crate) use legacy_model_mismatch_warning::LegacyModelMismatchWarning; pub(crate) use legacy_unified_exec_process_limit_warning::LegacyUnifiedExecProcessLimitWarning; pub use memory::MemoryContextFragment; +pub(crate) use model_instructions::ModelInstructions; pub(crate) use model_switch_instructions::ModelSwitchInstructions; pub(crate) use multi_agent_mode_instructions::MultiAgentModeInstructions; pub(crate) use multi_agent_usage_hint::MultiAgentUsageHint; diff --git a/codex-rs/core/src/context/model_instructions.rs b/codex-rs/core/src/context/model_instructions.rs new file mode 100644 index 0000000000..199c25ebd8 --- /dev/null +++ b/codex-rs/core/src/context/model_instructions.rs @@ -0,0 +1,52 @@ +//! A single model-context fragment for independent model and instruction changes. + +use super::ContextualUserFragment; +use codex_protocol::models::ContentItemKind; + +const REPLACEMENT_NOTICE: &str = + "The following instructions replace the previous base instructions:"; +const REMOVAL_NOTICE: &str = + "The previous base instructions no longer apply. No replacement base instructions are active."; + +#[derive(Clone, Debug, PartialEq)] +pub(crate) struct ModelInstructions { + pub(crate) model: Option, + /// None leaves instructions unchanged; Some replaces them, including an empty value. + pub(crate) instructions: Option, +} + +impl ContextualUserFragment for ModelInstructions { + fn content_kind(&self) -> ContentItemKind { + ContentItemKind("model.instructions".to_string()) + } + + fn role(&self) -> &'static str { + "developer" + } + + fn markers(&self) -> (&'static str, &'static str) { + Self::type_markers() + } + + fn type_markers() -> (&'static str, &'static str) { + ("", "") + } + + fn body(&self) -> String { + let mut body = String::from("\n"); + if let Some(model) = &self.model { + body.push_str(&format!("The current model is {model}.\n")); + } + match &self.instructions { + Some(instructions) if instructions.is_empty() => { + body.push_str(REMOVAL_NOTICE); + body.push('\n'); + } + Some(instructions) => { + body.push_str(&format!("{REPLACEMENT_NOTICE}\n\n{instructions}\n")); + } + None => {} + } + body + } +} diff --git a/codex-rs/core/src/context/world_state/base_instructions.rs b/codex-rs/core/src/context/world_state/base_instructions.rs deleted file mode 100644 index ac79a45425..0000000000 --- a/codex-rs/core/src/context/world_state/base_instructions.rs +++ /dev/null @@ -1,28 +0,0 @@ -//! Records fixed base instructions alongside tool declarations at the start of each window. - -use super::PreviousSectionState; -use super::SectionTransition; -use super::WorldStateHash; -use super::WorldStateSection; -use super::WorldStateUpdate; -use crate::context::BaseInstructionsFragment; - -pub(crate) struct BaseInstructionsState(pub(crate) String); - -impl WorldStateSection for BaseInstructionsState { - const ID: &'static str = "base_instructions"; - type Snapshot = WorldStateHash; - - fn render_diff( - &self, - previous: PreviousSectionState<'_, Self::Snapshot>, - ) -> SectionTransition { - let fragment = BaseInstructionsFragment(self.0.clone()); - let hash = WorldStateHash::from_fragment(&fragment); - let updates = WorldStateUpdate::optional_prefix_boxed_fragment( - (matches!(previous, PreviousSectionState::Absent) && !self.0.is_empty()) - .then(|| Box::new(fragment) as _), - ); - (Some(hash), updates) - } -} diff --git a/codex-rs/core/src/context/world_state/mod.rs b/codex-rs/core/src/context/world_state/mod.rs index 6a23a30c15..aff148f7e5 100644 --- a/codex-rs/core/src/context/world_state/mod.rs +++ b/codex-rs/core/src/context/world_state/mod.rs @@ -1,6 +1,5 @@ mod agents_md; mod apps_instructions; -mod base_instructions; mod collaboration_mode; mod compact_permissions; mod context_window_guidance; @@ -40,7 +39,6 @@ use std::fmt; pub(crate) use agents_md::AgentsMdState; pub(crate) use apps_instructions::AppsInstructionsState; -pub(crate) use base_instructions::BaseInstructionsState; pub(crate) use collaboration_mode::CollaborationModeState; pub(crate) use compact_permissions::CompactPermissionsState; pub(crate) use context_window_guidance::ContextWindowGuidanceState; @@ -108,18 +106,6 @@ impl WorldStateUpdate { .collect() } - pub(crate) fn optional_prefix_boxed_fragment( - fragment: Option>, - ) -> Vec { - fragment - .into_iter() - .map(|fragment| Self { - placement: Placement::Prefix, - content: WorldStateUpdateContent::Fragment(fragment), - }) - .collect() - } - pub(crate) fn standalone(mut self) -> Self { self.placement = Placement::Standalone; self @@ -195,7 +181,7 @@ impl ErasedWorldStateSection for S { } fn has_retained_fragment_matcher(&self) -> bool { - S::has_retained_fragment_matcher() + WorldStateSection::has_current_retained_fragment_matcher(self) } fn matches_retained_fragment(&self, role: &str, text: &str) -> bool { @@ -345,6 +331,11 @@ pub(crate) trait WorldStateSection: Send + Sync + 'static { false } + /// Whether this instance requires its rendered fragment to survive in history. + fn has_current_retained_fragment_matcher(&self) -> bool { + Self::has_retained_fragment_matcher() + } + /// Recognizes this section's rendered fragment in retained model history. fn matches_retained_fragment(_role: &str, _text: &str) -> bool { false diff --git a/codex-rs/core/src/context/world_state/model.rs b/codex-rs/core/src/context/world_state/model.rs index 2a15a61fd8..08461e6db2 100644 --- a/codex-rs/core/src/context/world_state/model.rs +++ b/codex-rs/core/src/context/world_state/model.rs @@ -1,14 +1,35 @@ +//! Owns model context in both window modes. Ordinary windows retain model-switch behavior; +//! incremental windows record the base and refresh instructions without replacing their prefix. + use super::PreviousSectionState; use super::SectionTransition; +use super::WorldStateHash; use super::WorldStateSection; use super::WorldStateUpdate; +use crate::context::BaseInstructionsFragment; use crate::context::ContextualUserFragment; +use crate::context::ModelInstructions; use crate::context::ModelSwitchInstructions; +use serde::Deserialize; +use serde::Serialize; -/// Model identity and the instructions needed when that identity changes. +/// Ordinary windows and older rollouts record only the model. +#[derive(Clone, Debug, Deserialize, PartialEq, Eq, Serialize)] +#[serde(untagged)] +pub(crate) enum ModelInstructionsSnapshot { + WithInstructions { + model: String, + instructions: WorldStateHash, + }, + ModelOnly(String), +} + +/// Model identity and instructions relevant to the current window. #[derive(Clone, Debug)] pub(crate) struct ModelInstructionsState { - model: String, + /// The current tracking format is chosen by window mode, independently of saved state. + snapshot: ModelInstructionsSnapshot, + /// Ordinary-window fallback when the saved model snapshot is unavailable. previous_model: Option, instructions: String, } @@ -16,23 +37,36 @@ pub(crate) struct ModelInstructionsState { impl ModelInstructionsState { pub(crate) fn new(model: &str, previous_model: Option<&str>, instructions: String) -> Self { Self { - model: model.to_string(), + snapshot: ModelInstructionsSnapshot::ModelOnly(model.to_string()), previous_model: previous_model.map(str::to_string), instructions, } } + + pub(crate) fn for_incremental_tools(model: &str, instructions: String) -> Self { + let instructions = BaseInstructionsFragment(instructions); + let instructions_hash = WorldStateHash::from_fragment(&instructions); + Self { + snapshot: ModelInstructionsSnapshot::WithInstructions { + model: model.to_string(), + instructions: instructions_hash, + }, + previous_model: None, + instructions: instructions.0, + } + } } impl WorldStateSection for ModelInstructionsState { const ID: &'static str = "model"; - type Snapshot = String; + type Snapshot = ModelInstructionsSnapshot; fn matches_legacy_fragment(role: &str, text: &str) -> bool { role == "developer" && ModelSwitchInstructions::matches_text(text) } - fn has_retained_fragment_matcher() -> bool { - true + fn has_current_retained_fragment_matcher(&self) -> bool { + matches!(self.snapshot, ModelInstructionsSnapshot::ModelOnly(_)) } fn matches_retained_fragment(role: &str, text: &str) -> bool { @@ -43,22 +77,59 @@ impl WorldStateSection for ModelInstructionsState { &self, previous: PreviousSectionState<'_, Self::Snapshot>, ) -> SectionTransition { - let current = self.model.clone(); - let model_changed = match previous { - PreviousSectionState::Known(previous) => previous != &self.model, - PreviousSectionState::Unknown | PreviousSectionState::Absent => self - .previous_model - .as_deref() - .is_some_and(|previous| previous != self.model), + let (model, instructions_hash) = match &self.snapshot { + ModelInstructionsSnapshot::ModelOnly(model) => (model, None), + ModelInstructionsSnapshot::WithInstructions { + model, + instructions, + } => (model, Some(instructions)), }; - - let fragment: Option> = + if instructions_hash.is_some() && !matches!(previous, PreviousSectionState::Known(_)) { + // Like tools, reassert full setup when the comparison state is missing. + if self.instructions.is_empty() { + return (Some(self.snapshot.clone()), Vec::new()); + } + return ( + Some(self.snapshot.clone()), + vec![WorldStateUpdate::prefix_item(ContextualUserFragment::into( + BaseInstructionsFragment(self.instructions.clone()), + ))], + ); + } + let (previous_model, previous_instructions_hash) = match previous { + PreviousSectionState::Known(ModelInstructionsSnapshot::ModelOnly(model)) => { + (Some(model), None) + } + PreviousSectionState::Known(ModelInstructionsSnapshot::WithInstructions { + model, + instructions, + }) if instructions_hash.is_some() => (Some(model), Some(instructions)), + // Ordinary windows keep their fallback for an unrecognized incremental snapshot. + PreviousSectionState::Known(ModelInstructionsSnapshot::WithInstructions { .. }) + | PreviousSectionState::Unknown + | PreviousSectionState::Absent => (self.previous_model.as_ref(), None), + }; + let model_changed = previous_model.is_some_and(|previous| previous != model); + // Legacy model-only snapshots refresh instructions only when the model changes. + let instructions_changed = instructions_hash.is_some_and(|current| { + previous_instructions_hash.map_or(model_changed, |previous| previous != current) + }); + if instructions_hash.is_some() && !model_changed && !instructions_changed { + return (None, Vec::new()); + } + let fragment: Option> = if instructions_hash.is_some() { + Some(Box::new(ModelInstructions { + model: model_changed.then(|| model.clone()), + instructions: instructions_changed.then(|| self.instructions.clone()), + })) + } else { (model_changed && !self.instructions.is_empty()).then(|| { Box::new(ModelSwitchInstructions::new(self.instructions.clone())) as Box - }); + }) + }; ( - Some(current), + Some(self.snapshot.clone()), WorldStateUpdate::optional_standalone_boxed_fragment(fragment), ) } diff --git a/codex-rs/core/src/context/world_state/model_tests.rs b/codex-rs/core/src/context/world_state/model_tests.rs index f83cc5ba32..a303b40728 100644 --- a/codex-rs/core/src/context/world_state/model_tests.rs +++ b/codex-rs/core/src/context/world_state/model_tests.rs @@ -5,10 +5,18 @@ use pretty_assertions::assert_eq; #[test] fn model_change_renders_when_persisted_or_inferred_from_previous_turn() { let state = ModelInstructionsState::new("gpt-new", Some("gpt-old"), "instructions".into()); - let previous = "gpt-old".to_string(); + let previous = ModelInstructionsSnapshot::ModelOnly("gpt-old".to_string()); + let previous_incremental = serde_json::from_value(serde_json::json!({ + "model": "gpt-new", + "instructions": WorldStateHash::from_fragment(&BaseInstructionsFragment( + "instructions".into(), + )), + })) + .unwrap(); for previous in [ PreviousSectionState::Known(&previous), + PreviousSectionState::Known(&previous_incremental), PreviousSectionState::Unknown, PreviousSectionState::Absent, ] { @@ -26,14 +34,14 @@ fn model_change_renders_when_persisted_or_inferred_from_previous_turn() { #[test] fn unchanged_model_does_not_render() { let state = ModelInstructionsState::new("gpt-test", Some("gpt-test"), "instructions".into()); - let previous = "gpt-test".to_string(); + let previous = ModelInstructionsSnapshot::ModelOnly("gpt-test".to_string()); - assert!( - state - .render_fragment_diff(PreviousSectionState::Known(&previous)) - .1 - .is_none() + let (snapshot, fragment) = state.render_fragment_diff(PreviousSectionState::Known(&previous)); + assert_eq!( + serde_json::to_value(snapshot).unwrap(), + serde_json::json!("gpt-test") ); + assert!(fragment.is_none()); assert!( state .render_fragment_diff(PreviousSectionState::Absent) @@ -41,3 +49,34 @@ fn unchanged_model_does_not_render() { .is_none() ); } + +#[test] +fn legacy_model_snapshot_keeps_its_window_until_model_change_or_compaction() { + let legacy: ModelInstructionsSnapshot = serde_json::from_str("\"gpt-old\"").unwrap(); + let same_model = ModelInstructionsState::for_incremental_tools("gpt-old", "new prompt".into()); + let (snapshot, updates) = same_model.render_diff(PreviousSectionState::Known(&legacy)); + assert!(snapshot.is_none()); + assert!(updates.is_empty()); + + let new_model = ModelInstructionsState::for_incremental_tools("gpt-new", "new prompt".into()); + let (snapshot, updates) = new_model.render_diff(PreviousSectionState::Known(&legacy)); + assert!(snapshot.is_some()); + assert_eq!(updates.len(), 1); + + for previous in [PreviousSectionState::Absent, PreviousSectionState::Unknown] { + let (snapshot, updates) = same_model.render_diff(previous); + assert_eq!( + crate::context_manager::updates::merge_world_state_updates(updates), + vec![ContextualUserFragment::into(BaseInstructionsFragment( + "new prompt".into() + ))] + ); + let snapshot = snapshot.unwrap(); + assert!( + same_model + .render_diff(PreviousSectionState::Known(&snapshot)) + .1 + .is_empty() + ); + } +} diff --git a/codex-rs/core/src/context_manager/history.rs b/codex-rs/core/src/context_manager/history.rs index db1c0e4096..f39be3fb41 100644 --- a/codex-rs/core/src/context_manager/history.rs +++ b/codex-rs/core/src/context_manager/history.rs @@ -662,9 +662,11 @@ impl ContextManager { &self, base_instructions: &BaseInstructions, ) -> Option { - // Incremental windows already account for their recorded base instructions below. - let has_recorded_instructions = - self.raw_items().any(BaseInstructionsFragment::matches_item); + // Incremental windows supply instructions through history, including an empty base. + let has_recorded_instructions = self.has_tool_declarations() + || self + .raw_items() + .any(|item| BaseInstructionsFragment::text_from_item(item).is_some()); let base_tokens = if has_recorded_instructions { 0 } else { diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 427457042f..14a88521ba 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -11388,6 +11388,10 @@ async fn build_initial_context_uses_retained_step_after_model_change() { let messages = model_info.model_messages.as_mut().unwrap(); messages.instructions_template = Some("A instructions".to_string()); }); + session.state.lock().await.base_instructions_provenance = + Some(codex_protocol::models::BaseInstructionsProvenance::Model { + model: "base-model".to_string(), + }); session .set_previous_turn_settings(Some(PreviousTurnSettings { model: "base-model".to_string(), diff --git a/codex-rs/core/src/session/world_state.rs b/codex-rs/core/src/session/world_state.rs index 3426f76a54..7b33283897 100644 --- a/codex-rs/core/src/session/world_state.rs +++ b/codex-rs/core/src/session/world_state.rs @@ -6,7 +6,6 @@ use crate::connectors; use crate::context::TokenBudgetContext; use crate::context::world_state::AgentsMdState; use crate::context::world_state::AppsInstructionsState; -use crate::context::world_state::BaseInstructionsState; use crate::context::world_state::CollaborationModeState; use crate::context::world_state::CompactPermissionsState; use crate::context::world_state::ContextWindowGuidanceState; @@ -76,35 +75,54 @@ impl Session { selected_capability_root_count = step_context.selected_capability_roots.len(), "building step world state" ); - let model_instructions = render_model_instructions(model_info); - let model_instructions = if !turn_context.config.update_plan_enabled - && turn_context.config.model_catalog.is_none() - && (turn_context.config.base_instructions.is_none() - || matches!( - turn_context.config.base_instructions_provenance, - Some(BaseInstructionsProvenance::Model { .. }) - )) { - codex_prompts::without_update_plan_instructions(&model_instructions) - } else { - model_instructions + let render_current_instructions = || { + let instructions = render_model_instructions(model_info); + if !turn_context.config.update_plan_enabled + && turn_context.config.model_catalog.is_none() + && (incremental_tools + || turn_context.config.base_instructions.is_none() + || matches!( + turn_context.config.base_instructions_provenance, + Some(BaseInstructionsProvenance::Model { .. }) + )) + { + codex_prompts::without_update_plan_instructions(&instructions) + } else { + instructions + } }; - let base_instructions = self.get_prompt_base_instructions().await.text; - let previous_model = { - let state = self.state.lock().await; - state - .previous_turn_settings() - .map(|previous| previous.model) - .or_else(|| { - state - .base_instructions_provenance - .as_ref() - .and_then(|provenance| match provenance { - BaseInstructionsProvenance::Model { model } => Some(model), - BaseInstructionsProvenance::Custom => None, - }) - .filter(|_| base_instructions != model_instructions) - .cloned() - }) + let model_state = if incremental_tools { + let base_instructions = self.get_base_instructions().await; + let instructions = match base_instructions.provenance { + Some(BaseInstructionsProvenance::Model { .. }) => render_current_instructions(), + Some(BaseInstructionsProvenance::Custom) | None => base_instructions.text, + }; + ModelInstructionsState::for_incremental_tools(&model_info.slug, instructions) + } else { + let model_instructions = render_current_instructions(); + let base_instructions = self.get_prompt_base_instructions().await.text; + let previous_model = { + let state = self.state.lock().await; + state + .previous_turn_settings() + .map(|previous| previous.model) + .or_else(|| { + state + .base_instructions_provenance + .as_ref() + .and_then(|provenance| match provenance { + BaseInstructionsProvenance::Model { model } => Some(model), + BaseInstructionsProvenance::Custom => None, + }) + .filter(|_| base_instructions != model_instructions) + .cloned() + }) + }; + ModelInstructionsState::new( + &model_info.slug, + previous_model.as_deref(), + model_instructions, + ) }; let environment_subagents = if turn_context.config.include_environment_context { match turn_context.multi_agent_version { @@ -152,13 +170,8 @@ impl Session { definitions, Some(Arc::clone(&extension_metrics)), )?); - world_state.add_section(BaseInstructionsState(base_instructions)); } - world_state.add_section(ModelInstructionsState::new( - &model_info.slug, - previous_model.as_deref(), - model_instructions, - )); + world_state.add_section(model_state); let token_budget_enabled = turn_context.config.features.enabled(Feature::TokenBudget) && step_context .settings diff --git a/codex-rs/core/tests/suite/base_instructions.rs b/codex-rs/core/tests/suite/base_instructions.rs new file mode 100644 index 0000000000..8769dd313a --- /dev/null +++ b/codex-rs/core/tests/suite/base_instructions.rs @@ -0,0 +1,350 @@ +//! Base instructions stay fixed within a context window and refresh when a new window opens. + +use anyhow::Result; +use codex_core::ForkSnapshot; +use codex_core::StartThreadOptions; +use codex_core::TurnInputRequest; +use codex_features::Feature; +use codex_login::CodexAuth; +use codex_models_manager::manager::RefreshStrategy; +use codex_models_manager::model_info::model_info_from_slug; +use codex_protocol::openai_models::ModelInfo; +use codex_protocol::openai_models::ModelVisibility; +use codex_protocol::openai_models::ModelsResponse; +use codex_protocol::protocol::EventMsg::TurnComplete; +use codex_protocol::protocol::Op; +use codex_protocol::protocol::ThreadHistoryMode; +use codex_protocol::protocol::ThreadSettingsOverrides; +use codex_protocol::user_input::UserInput; +use core_test_support::responses; +use core_test_support::responses::ResponsesRequest as Request; +use core_test_support::skip_if_no_network; +use core_test_support::skip_if_remote; +use core_test_support::test_codex::TestCodex; +use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; +use pretty_assertions::assert_eq; +use serde_json::json; +use test_case::test_case; +use wiremock::MockServer; + +const MODEL: &str = "gpt-5.4"; +const SWITCHED_MODEL: &str = "gpt-5.2"; +const ORIGINAL: &str = "original managed base instructions"; +const UPDATED: &str = "updated managed base instructions"; +const SWITCHED: &str = "switched managed base instructions"; + +fn managed_model(slug: &str, instructions: &str) -> ModelInfo { + let mut model = model_info_from_slug(slug); + model.use_responses_lite = true; + model.visibility = ModelVisibility::List; + model + .model_messages + .as_mut() + .expect("model messages") + .instructions_template = Some(instructions.to_string()); + model +} + +async fn refresh_catalog(test: &TestCodex, server: &MockServer, model: &str, instructions: &str) { + let etag = format!("\"{model}-{}\"", instructions.len()); + let models = vec![managed_model(model, instructions)]; + responses::mount_models_once_with_etag(server, ModelsResponse { models }, &etag).await; + let client_factory = codex_core::test_support::default_http_client_factory(); + test.thread_manager + .get_models_manager() + .list_models(RefreshStrategy::Online, client_factory) + .await; +} + +async fn send_turn(test: &TestCodex, server: &MockServer, text: &str) -> Result { + let response = responses::mount_sse_once(server, responses::sse_completed(text)).await; + test.submit_text_turn(text).await?; + Ok(response.single_request()) +} + +async fn switch_model(test: &TestCodex, server: &MockServer, model: &str) -> Result { + let overrides = ThreadSettingsOverrides { + model: Some(model.to_string()), + ..Default::default() + }; + core_test_support::submit_thread_settings(&test.codex, overrides).await?; + send_turn(test, server, "switch models").await +} + +async fn compact(test: &TestCodex, server: &MockServer) -> Result<()> { + let events = vec![ + json!({"type": "response.output_item.done", "item": { + "type": "compaction", "encrypted_content": "summary" + }}), + responses::ev_completed("compaction"), + ]; + responses::mount_sse_once(server, responses::sse(events)).await; + test.codex.submit(Op::Compact).await?; + wait_for_event(&test.codex, |event| matches!(event, TurnComplete(_))).await; + Ok(()) +} + +fn model_updates(request: &Request) -> Vec { + request + .message_input_texts("developer") + .into_iter() + .filter(|text| text.contains("")) + .collect() +} + +fn assert_request_base(request: &Request, base: &str) { + let developer = request.message_input_texts("developer"); + assert!(request.body_json().get("instructions").is_none()); + if !base.is_empty() { + let first_message = request + .input() + .into_iter() + .find(|item| item["type"] != "additional_tools") + .expect("base instruction prefix"); + assert_eq!(first_message["content"][0]["text"], base); + } + assert_eq!( + developer + .iter() + .filter(|text| text.as_str() == base) + .count(), + usize::from(!base.is_empty()) + ); +} + +#[test_case(true, None; "managed incremental instructions")] +#[test_case(false, None; "flag disabled")] +#[test_case(true, Some("user-supplied instructions"); "custom instructions")] +#[test_case(true, Some(""); "empty custom instructions")] +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn catalog_and_model_changes_preserve_the_window_base( + incremental_tools: bool, + custom_instructions: Option<&'static str>, +) -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = MockServer::start().await; + let models = vec![managed_model(MODEL, ORIGINAL)]; + responses::mount_models_once_with_etag(&server, ModelsResponse { models }, "\"v1\"").await; + let test = test_codex() + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_model(MODEL) + .with_config(move |config| { + config.base_instructions = custom_instructions.map(str::to_string); + if incremental_tools { + config + .features + .enable(Feature::IncrementalTools) + .expect("enable incremental tools"); + } else { + config + .features + .disable(Feature::IncrementalTools) + .expect("disable incremental tools"); + } + }) + .build_with_auto_env(&server) + .await?; + let first_request = send_turn(&test, &server, "first turn").await?; + assert!(model_updates(&first_request).is_empty()); + + refresh_catalog(&test, &server, MODEL, UPDATED).await; + let updated_request = send_turn(&test, &server, "use refreshed instructions").await?; + let mut updates = Vec::new(); + if incremental_tools && custom_instructions.is_none() { + updates.push(format!("\nThe following instructions replace the previous base instructions:\n\n{UPDATED}\n")); + } + assert_eq!(model_updates(&updated_request), updates); + assert_eq!( + updated_request.has_content_kinds(&["model.instructions"]), + incremental_tools && custom_instructions.is_none() + ); + assert!(updated_request.input().starts_with(&first_request.input())); + let unchanged_request = send_turn(&test, &server, "keep refreshed instructions").await?; + assert_eq!(model_updates(&unchanged_request), updates); + + refresh_catalog(&test, &server, MODEL, "").await; + let cleared_request = send_turn(&test, &server, "clear managed instructions").await?; + if incremental_tools && custom_instructions.is_none() { + updates.push("\nThe previous base instructions no longer apply. No replacement base instructions are active.\n".to_string()); + } + assert_eq!(model_updates(&cleared_request), updates); + + for request in [ + first_request, + updated_request, + unchanged_request, + cleared_request, + ] { + assert_request_base(&request, custom_instructions.unwrap_or(ORIGINAL)); + } + + compact(&test, &server).await?; + let compacted_request = send_turn(&test, &server, "keep cleared instructions").await?; + if !incremental_tools { + assert_request_base(&compacted_request, ORIGINAL); + assert!(model_updates(&compacted_request).is_empty()); + return Ok(()); + } + assert_request_base(&compacted_request, custom_instructions.unwrap_or_default()); + assert!(model_updates(&compacted_request).is_empty()); + assert!(!compacted_request.body_contains_text(ORIGINAL)); + assert!(!compacted_request.body_contains_text(UPDATED)); + + refresh_catalog(&test, &server, SWITCHED_MODEL, SWITCHED).await; + let switched_request = switch_model(&test, &server, SWITCHED_MODEL).await?; + let updates = vec![match custom_instructions { + None => format!( + "\nThe current model is {SWITCHED_MODEL}.\nThe following instructions replace the previous base instructions:\n\n{SWITCHED}\n" + ), + Some(_) => format!( + "\nThe current model is {SWITCHED_MODEL}.\n" + ), + }]; + assert_eq!(model_updates(&switched_request), updates); + assert_request_base(&switched_request, custom_instructions.unwrap_or_default()); + + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn rebuilding_retained_context_reasserts_current_instructions() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_remote!(Ok(()), "the rollout fixture is edited on the host"); + + let server = MockServer::start().await; + let builder = |instructions: &'static str| { + test_codex() + .with_model(MODEL) + .with_history_mode(Some(ThreadHistoryMode::Legacy)) + .with_config(move |config| { + config.model_catalog = Some(ModelsResponse { + models: vec![managed_model(MODEL, instructions)], + }); + config + .features + .enable(Feature::IncrementalTools) + .expect("enable incremental tools"); + }) + }; + let initial = builder(ORIGINAL).build_with_auto_env(&server).await?; + let first = send_turn(&initial, &server, "start the conversation").await?; + initial.codex.shutdown_and_wait().await?; + let path = initial.codex.rollout_path().expect("rollout path"); + + // A missing turn-context record forces full reassertion within the retained window. + let mut lines = Vec::new(); + for line in std::fs::read_to_string(&path)?.lines() { + let item: serde_json::Value = serde_json::from_str(line)?; + if item["type"] == "turn_context" { + continue; + } + lines.push(line.to_string()); + } + std::fs::write(&path, format!("{}\n", lines.join("\n")))?; + + let resumed = builder(UPDATED) + .resume(&server, initial.home.clone(), path) + .await?; + let request = send_turn(&resumed, &server, "resume with updated instructions").await?; + assert!(request.input().starts_with(&first.input())); + assert_request_base(&request, ORIGINAL); + assert_eq!( + request + .message_input_texts("developer") + .into_iter() + .filter(|text| text.as_str() == ORIGINAL || text.as_str() == UPDATED) + .collect::>(), + vec![ORIGINAL, UPDATED], + ); + Ok(()) +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn model_switches_refresh_the_window_base_across_compaction_and_restart() -> Result<()> { + skip_if_no_network!(Ok(())); + + const SAME_PROMPT_MODEL: &str = "gpt-5.1"; + let server = MockServer::start().await; + let builder = || { + test_codex().with_model(MODEL).with_config(move |config| { + config + .features + .enable(Feature::IncrementalTools) + .expect("enable incremental tools"); + config.model_catalog = Some(ModelsResponse { + models: vec![ + managed_model(MODEL, ORIGINAL), + managed_model(SWITCHED_MODEL, SWITCHED), + managed_model(SAME_PROMPT_MODEL, SWITCHED), + ], + }); + }) + }; + let test = builder().build_with_auto_env(&server).await?; + let first_request = send_turn(&test, &server, "first turn").await?; + assert!(model_updates(&first_request).is_empty()); + let switched_request = switch_model(&test, &server, SWITCHED_MODEL).await?; + let mut expected_updates = vec![format!( + "\nThe current model is {SWITCHED_MODEL}.\nThe following instructions replace the previous base instructions:\n\n{SWITCHED}\n" + )]; + assert_eq!(model_updates(&switched_request), expected_updates); + + let same_prompt_request = switch_model(&test, &server, SAME_PROMPT_MODEL).await?; + expected_updates.push(format!( + "\nThe current model is {SAME_PROMPT_MODEL}.\n" + )); + assert_eq!(model_updates(&same_prompt_request), expected_updates); + + compact(&test, &server).await?; + let compacted_request = send_turn(&test, &server, "continue after compaction").await?; + assert_request_base(&compacted_request, SWITCHED); + assert!(model_updates(&compacted_request).is_empty()); + assert!(!compacted_request.body_contains_text(ORIGINAL)); + test.codex.flush_rollout().await?; + let mut fork_config = test.config.clone(); + fork_config.model = Some(SAME_PROMPT_MODEL.to_string()); + let fork = test + .thread_manager + .fork_legacy_thread( + ForkSnapshot::Interrupted, + StartThreadOptions::new(fork_config), + test.codex.rollout_path().expect("rollout path"), + ) + .await?; + let fork_response = + responses::mount_sse_once(&server, responses::sse_completed("fork after compaction")).await; + fork.thread + .start_or_steer_turn(TurnInputRequest::user_input(vec![UserInput::Text { + text: "continue in fork".into(), + text_elements: Vec::new(), + }])) + .await?; + wait_for_event(&fork.thread, |event| matches!(event, TurnComplete(_))).await; + let fork_request = fork_response.single_request(); + assert_request_base(&fork_request, SWITCHED); + assert!(model_updates(&fork_request).is_empty()); + assert!(!fork_request.body_contains_text(ORIGINAL)); + + let resumed = builder() + .with_model(SAME_PROMPT_MODEL) + .restart(&server, &test) + .await?; + let resumed_request = send_turn(&resumed, &server, "continue after restart").await?; + assert_request_base(&resumed_request, SWITCHED); + assert!(model_updates(&resumed_request).is_empty()); + assert!(!resumed_request.body_contains_text(ORIGINAL)); + let resumed_input = resumed_request.input(); + let compacted_input = compacted_request.input(); + if test.executor_environment().exec_server_url().is_some() { + assert!(resumed_input[1..].starts_with(&compacted_input[1..])); + } else { + assert!(resumed_input.starts_with(&compacted_input)); + } + + for request in [first_request, switched_request, same_prompt_request] { + assert_request_base(&request, ORIGINAL); + } + Ok(()) +} diff --git a/codex-rs/core/tests/suite/mod.rs b/codex-rs/core/tests/suite/mod.rs index bc2f075304..d64ef3a06d 100644 --- a/codex-rs/core/tests/suite/mod.rs +++ b/codex-rs/core/tests/suite/mod.rs @@ -55,6 +55,7 @@ mod approvals; mod audio_truncation; mod auth_recovery_policy; mod auto_review; +mod base_instructions; #[path = "bedrock_multi_agent_tests.rs"] mod bedrock_multi_agent; #[path = "bedrock_service_tier_tests.rs"] diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index cfad8149ad..2f55a0c935 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -105,6 +105,9 @@ mod incremental_tools; #[path = "scenarios_incremental_tools_resume.rs"] mod incremental_tools_resume; +#[path = "scenarios_model_instructions.rs"] +mod model_instructions; + #[path = "scenarios_code_mode_settled_helpers_tests.rs"] mod code_mode_settled_helpers; diff --git a/codex-rs/core/tests/suite/scenarios_incremental_tools.rs b/codex-rs/core/tests/suite/scenarios_incremental_tools.rs index 16e7f58c02..b434a16845 100644 --- a/codex-rs/core/tests/suite/scenarios_incremental_tools.rs +++ b/codex-rs/core/tests/suite/scenarios_incremental_tools.rs @@ -6,6 +6,8 @@ use codex_protocol::openai_models::CodeModeToolMessages; use codex_protocol::openai_models::IncrementalToolMessages; use codex_protocol::openai_models::ToolMessage; use codex_protocol::openai_models::ToolMode; +use codex_protocol::protocol::EventMsg; +use codex_protocol::protocol::Op; use codex_protocol::protocol::ThreadSettingsOverrides; use core_test_support::context_snapshot; use core_test_support::context_snapshot::ContextSnapshotOptions; @@ -13,7 +15,9 @@ use core_test_support::responses; use core_test_support::skip_if_no_network; use core_test_support::skip_if_wine_exec; use core_test_support::test_codex::test_codex; +use core_test_support::wait_for_event; use core_test_support::wait_for_mcp_server; +use pretty_assertions::assert_eq; use serde_json::json; use test_case::test_case; @@ -30,8 +34,21 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( let server = responses::start_mock_server().await; let mock = responses::mount_sse_sequence( &server, - (1..=4) - .map(|index| responses::sse(vec![responses::ev_completed(&format!("resp-{index}"))])) + (1..=if use_responses_lite { 6 } else { 4 }) + .map(|index| { + let mut events = Vec::new(); + if index == 5 { + events.push(json!({ + "type": "response.output_item.done", + "item": { + "type": "compaction", + "encrypted_content": "INCREMENTAL_TOOLS_CHECKPOINT", + }, + })); + } + events.push(responses::ev_completed(&format!("resp-{index}"))); + responses::sse(events) + }) .collect(), ) .await; @@ -39,14 +56,24 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( .with_model_info_override("gpt-5.4", move |model| { model.use_responses_lite = use_responses_lite; model.tool_mode = Some(ToolMode::CodeMode); + if use_responses_lite { + model + .model_messages + .as_mut() + .expect("model messages") + .instructions_template = + Some("Use the available tools to help the user.".to_string()); + } }) - .with_config(|config| { + .with_config(move |config| { config .features .enable(Feature::IncrementalTools) .expect("enable incremental tools"); - config.base_instructions = - Some("Use the available tools to help the user.".to_string()); + if !use_responses_lite { + config.base_instructions = + Some("Use the available tools to help the user.".to_string()); + } config.code_mode.disable_in_process_fallback = true; let catalog = config.model_catalog.as_mut().expect("model catalog"); let mut updated = catalog @@ -56,6 +83,13 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( .expect("source model") .clone(); updated.slug = "updated-tools-model".to_string(); + if use_responses_lite { + updated + .model_messages + .as_mut() + .expect("model messages") + .instructions_template = Some("Review results before responding.".to_string()); + } updated .model_messages .get_or_insert_default() @@ -86,8 +120,29 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( .await?; test.submit_text_turn("Continue with the same updated tools.") .await?; + if use_responses_lite { + test.codex.submit(Op::Compact).await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + test.submit_text_turn("Continue after compaction.").await?; + } let requests = mock.requests(); + if use_responses_lite { + for request in &requests[..5] { + assert_eq!( + request.instructions_text(), + "Use the available tools to help the user." + ); + } + assert!(!requests[5].body_contains_text("Use the available tools to help the user.")); + assert_eq!( + requests[5].instructions_text(), + "Review results before responding." + ); + } insta::assert_snapshot!( if use_responses_lite { "incremental_tools" @@ -96,7 +151,7 @@ async fn incremental_tools_append_changed_catalog_without_rewriting_history( }, context_snapshot::format_request_history_snapshot( if use_responses_lite { - "Tool definitions enter history in one batch; a catalog change appends one developer notice before only the changed exec definition, without modifying namespace descriptions. The next unchanged turn appends only user input." + "Tool definitions precede the initial base instructions. A model switch appends one developer notice, changed tools, and replacement instructions without rewriting the prefix; compaction starts a new window with the updated base." } else { "Regular Responses requests carry the full current tool catalog without an incremental notice; changed execution instructions replace the catalog on subsequent requests." }, diff --git a/codex-rs/core/tests/suite/scenarios_model_instructions.rs b/codex-rs/core/tests/suite/scenarios_model_instructions.rs new file mode 100644 index 0000000000..2e16542430 --- /dev/null +++ b/codex-rs/core/tests/suite/scenarios_model_instructions.rs @@ -0,0 +1,104 @@ +//! Instruction refresh and compaction with an unchanged model and tool catalog. + +use super::*; +use codex_models_manager::manager::RefreshStrategy; +use codex_models_manager::model_info::model_info_from_slug; +use codex_protocol::openai_models::ModelVisibility; +use codex_protocol::openai_models::ModelsResponse; +use core_test_support::responses; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn catalog_refresh_replaces_instructions_and_compaction_installs_latest_base() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = wiremock::MockServer::start().await; + let mut model = model_info_from_slug("gpt-5.4"); + model.use_responses_lite = true; + model.visibility = ModelVisibility::List; + model + .model_messages + .get_or_insert_default() + .instructions_template = Some("Help the user complete their task.".to_string()); + responses::mount_models_once_with_etag( + &server, + ModelsResponse { + models: vec![model.clone()], + }, + "\"instructions-v1\"", + ) + .await; + let mock = mount_sse_sequence( + &server, + (1..=5) + .map(|index| { + let mut events = Vec::new(); + if index == 4 { + events.push(json!({ + "type": "response.output_item.done", + "item": { + "type": "compaction", + "encrypted_content": "MODEL_INSTRUCTIONS_CHECKPOINT", + }, + })); + } + events.push(ev_completed(&format!("resp-{index}"))); + sse(events) + }) + .collect(), + ) + .await; + let test = test_codex() + .with_model(&model.slug) + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(|config| { + config.workspace_roots = vec![config.cwd.clone()]; + config + .features + .enable(Feature::IncrementalTools) + .expect("enable incremental tools"); + }) + .build_with_auto_env(&server) + .await?; + test.submit_turn("Begin work.").await?; + + model + .model_messages + .get_or_insert_default() + .instructions_template = Some("Check your work before responding.".to_string()); + responses::mount_models_once_with_etag( + &server, + ModelsResponse { + models: vec![model], + }, + "\"instructions-v2\"", + ) + .await; + test.thread_manager + .get_models_manager() + .list_models( + RefreshStrategy::Online, + codex_core::test_support::default_http_client_factory(), + ) + .await; + test.submit_turn("Continue with refreshed instructions.") + .await?; + test.submit_turn("Continue without further changes.") + .await?; + test.codex.submit(Op::Compact).await?; + wait_for_event(&test.codex, |event| { + assert!(!matches!(event, EventMsg::Error(_)), "{event:?}"); + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + test.submit_turn("Continue after compaction.").await?; + + insta::assert_snapshot!( + "incremental_model_instructions", + context_snapshot::format_request_history_snapshot( + "A same-model catalog refresh appends replacement instructions once without changing tools or the prefix. Compaction starts a new window with the latest base.", + &mock.requests(), + &ContextSnapshotOptions::default().include_request_settings(), + ) + ); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__incremental_tools__incremental_tools.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__incremental_tools__incremental_tools.snap index 73d233a689..6e02804c35 100644 --- a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__incremental_tools__incremental_tools.snap +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__incremental_tools__incremental_tools.snap @@ -1,8 +1,8 @@ --- source: core/tests/suite/scenarios_incremental_tools.rs -expression: "context_snapshot::format_request_history_snapshot(\"Tool definitions enter history in one batch; a catalog change appends one developer notice before only the changed exec definition, without modifying namespace descriptions. The next unchanged turn appends only user input.\",\n&requests,\n&ContextSnapshotOptions::default().rewrite_known_segments().include_request_settings(),)" +expression: "context_snapshot::format_request_history_snapshot(if use_responses_lite\n{\n \"Tool definitions precede the initial base instructions. A model switch appends one developer notice, changed tools, and replacement instructions without rewriting the prefix; compaction starts a new window with the updated base.\"\n} else\n{\n \"Regular Responses requests carry the full current tool catalog without an incremental notice; changed execution instructions replace the catalog on subsequent requests.\"\n}, &requests,\n&ContextSnapshotOptions::default().rewrite_known_segments().include_request_settings(),)" --- -Scenario: Tool definitions enter history in one batch; a catalog change appends one developer notice before only the changed exec definition, without modifying namespace descriptions. The next unchanged turn appends only user input. +Scenario: Tool definitions precede the initial base instructions. A model switch appends one developer notice, changed tools, and replacement instructions without rewriting the prefix; compaction starts a new window with the updated base. ## Window 1 Settings: @@ -68,9 +68,47 @@ Settings: relative to window 1 - namespace/functions; hash=771cad2ff250d2f3 - custom/exec 08:message/developer: - + + The current model is updated-tools-model. + The following instructions replace the previous base instructions: + + Review results before responding. + 09:message/user: Continue with the updated execution instructions. -- request 4 (turn) -- 10:message/user: Continue with the same updated tools. +-- request 5 (compaction) -- +11:compaction_trigger + +## Window 3 (after request 5: input diverged at item 00) +Settings: same as window 2 +-- request 6 (turn) -- +00:additional_tools/developer (2; hash=1322c3a339378659): + - namespace/functions; hash=9d5d7a0adab05fef + - custom/exec + - function/wait + - function/exec_command + - function/write_stdin + - function/request_user_input + - custom/apply_patch + - function/view_image + - tool_search: # Tool discovery Searches over deferred tool metadata with BM25 and exposes matching tools for t...; args=[limit, query]; hash=400cd6ac61d524a9 +01:message/developer: + +02:message/user: + Begin work. +03:message/user: + Continue with the same tools. +04:message/user: + Continue with the updated execution instructions. +05:message/developer: + +06:message/user: + +07:message/user: + Continue with the same updated tools. +08:compaction:encrypted=true; chars=28; hash=e4a73156e0a498bc +09:message/user: + Continue after compaction. diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__model_instructions__incremental_model_instructions.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__model_instructions__incremental_model_instructions.snap new file mode 100644 index 0000000000..0a1d2ba5a5 --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__model_instructions__incremental_model_instructions.snap @@ -0,0 +1,100 @@ +--- +source: core/tests/suite/scenarios_model_instructions.rs +expression: "context_snapshot::format_request_history_snapshot(\"A same-model catalog refresh appends replacement instructions once without changing tools or the prefix. Compaction starts a new window with the latest base.\",\n&mock.requests(),\n&ContextSnapshotOptions::default().include_request_settings(),)" +--- +Scenario: A same-model catalog refresh appends replacement instructions once without changing tools or the prefix. Compaction starts a new window with the latest base. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + model: "gpt-5.4" + parallel_tool_calls: false + prompt_cache_key: "" + reasoning: {"context":"all_turns","summary":"auto"} + store: false + stream: true + tool_choice: "auto" +-- request 1 (turn) -- +00:additional_tools/developer (2; hash=946019c94afdd69a): + - namespace/functions; hash=ce01915b77cef9ee + - function/exec_command + - function/write_stdin + - function/request_user_input + - function/view_image + - namespace/multi_agent_v1: Tools for spawning and managing sub-agents.; hash=d492855cc7064c73 + - function/close_agent + - function/resume_agent + - function/send_input + - function/spawn_agent + - function/wait_agent +01:message/developer: + Help the user complete their task. +02:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `danger-full-access`...ll commands are permitted. Network access is enabled. [hash=c134061cebb94a1b] + Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. + +03:message/user: + + + + + + <...e="unrestricted" /> [hash=d40b2f45cb028888] + +04:message/user: + Begin work. +-- request 2 (turn) -- +05:message/developer: + + The following instructions replace the previous base instructions: + + Check your work before responding. + +06:message/user: + Continue with refreshed instructions. +-- request 3 (turn) -- +07:message/user: + Continue without further changes. +-- request 4 (compaction) -- +08:compaction_trigger + +## Window 2 (after request 4: input diverged at item 01) +Settings: same as window 1 +-- request 5 (turn) -- +00:additional_tools/developer (2; hash=946019c94afdd69a): + - namespace/functions; hash=ce01915b77cef9ee + - function/exec_command + - function/write_stdin + - function/request_user_input + - function/view_image + - namespace/multi_agent_v1: Tools for spawning and managing sub-agents.; hash=d492855cc7064c73 + - function/close_agent + - function/resume_agent + - function/send_input + - function/spawn_agent + - function/wait_agent +01:message/developer: + Check your work before responding. +02:message/user: + Begin work. +03:message/user: + Continue with refreshed instructions. +04:message/developer: + + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `danger-full-access`...ll commands are permitted. Network access is enabled. [hash=c134061cebb94a1b] + Approval policy is currently never. Do not provide the `sandbox_permissions` for any reason, commands will be rejected. + +05:message/user: + + + + + + <...e="unrestricted" /> [hash=d40b2f45cb028888] + +06:message/user: + Continue without further changes. +07:compaction:encrypted=true; chars=29; hash=a44318b8dfee766d +08:message/user: + Continue after compaction.