diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index c217537692..11e616e875 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -884,6 +884,9 @@ "browser_use_full_cdp_access": { "type": "boolean" }, + "catalog_apps_instructions": { + "type": "boolean" + }, "chronicle": { "type": "boolean" }, @@ -8010,6 +8013,9 @@ "browser_use_full_cdp_access": { "type": "boolean" }, + "catalog_apps_instructions": { + "type": "boolean" + }, "chronicle": { "type": "boolean" }, diff --git a/codex-rs/core/src/apps/render.rs b/codex-rs/core/src/apps/render.rs index 455cb6ecc7..c5c877a58d 100644 --- a/codex-rs/core/src/apps/render.rs +++ b/codex-rs/core/src/apps/render.rs @@ -9,7 +9,7 @@ pub(crate) fn render_apps_section(connectors: &[AppInfo]) -> Option { connectors .iter() .any(|connector| connector.is_accessible && connector.is_enabled) - .then(|| AppsInstructions.render().expect_text()) + .then(|| AppsInstructions::default().render().expect_text()) } #[cfg(test)] diff --git a/codex-rs/core/src/context/apps_instructions.rs b/codex-rs/core/src/context/apps_instructions.rs index e93058eebb..bbb22c53a4 100644 --- a/codex-rs/core/src/context/apps_instructions.rs +++ b/codex-rs/core/src/context/apps_instructions.rs @@ -1,6 +1,11 @@ +//! Renders bundled Apps guidance and catalog updates with the same developer provenance. +//! Older clients recognize only the bundled markers when recovering from retained history. + use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_protocol::protocol::APPS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::APPS_INSTRUCTIONS_OPEN_TAG; +use codex_protocol::protocol::CATALOG_APPS_INSTRUCTIONS_CLOSE_TAG; +use codex_protocol::protocol::CATALOG_APPS_INSTRUCTIONS_OPEN_TAG; use super::ContextualUserFragment; use codex_protocol::models::ContentItem; @@ -8,7 +13,17 @@ use codex_protocol::models::ContentItemKind; use codex_protocol::models::input_text; #[derive(Debug, Clone, PartialEq)] -pub(crate) struct AppsInstructions; +pub(crate) struct AppsInstructions(pub(crate) String); + +impl Default for AppsInstructions { + fn default() -> Self { + Self( + codex_prompts::ResolvedModelMessages::bundled() + .apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME) + .usage_instructions, + ) + } +} impl ContextualUserFragment for AppsInstructions { fn content_kind(&self) -> ContentItemKind { @@ -28,8 +43,34 @@ impl ContextualUserFragment for AppsInstructions { } fn body(&self) -> ContentItem { - input_text(format!( - "\n## Apps (Connectors)\nApps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{{connector_id}})`. Apps can also be implicitly triggered as long as the context suggests usage of available apps.\nAn app is equivalent to a set of MCP tools within the `{CODEX_APPS_MCP_SERVER_NAME}` MCP.\nAn installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool_search` tool. If `tool_search` is available, the apps that are searchable by `tools_search` will be listed by it.\nDo not additionally call list_mcp_resources or list_mcp_resource_templates for apps.\n" - )) + input_text(self.0.clone()) + } +} + +#[derive(Debug, Clone, PartialEq)] +pub(crate) struct CatalogAppsInstructions(pub(crate) String); + +impl ContextualUserFragment for CatalogAppsInstructions { + fn content_kind(&self) -> ContentItemKind { + ContentItemKind("apps.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) { + ( + CATALOG_APPS_INSTRUCTIONS_OPEN_TAG, + CATALOG_APPS_INSTRUCTIONS_CLOSE_TAG, + ) + } + + fn body(&self) -> ContentItem { + input_text(self.0.clone()) } } diff --git a/codex-rs/core/src/context/mod.rs b/codex-rs/core/src/context/mod.rs index 189e6dbb41..cd7b96b6ef 100644 --- a/codex-rs/core/src/context/mod.rs +++ b/codex-rs/core/src/context/mod.rs @@ -60,6 +60,7 @@ pub(crate) use approved_command_prefix_saved::APPROVED_COMMAND_PREFIX_SAVED_MESS pub(crate) use approved_command_prefix_saved::ApprovedCommandPrefixSaved; pub(crate) use approved_command_prefix_saved::SAVED_PREFIX_KIND; pub(crate) use apps_instructions::AppsInstructions; +pub(crate) use apps_instructions::CatalogAppsInstructions; pub(crate) use available_plugins_instructions::AvailablePluginsInstructions; pub(crate) use base_instructions::BaseInstructionsFragment; pub(crate) use codex_context_fragments::AdditionalContextDeveloperFragment; diff --git a/codex-rs/core/src/context/world_state/apps_instructions.rs b/codex-rs/core/src/context/world_state/apps_instructions.rs index 82076ee934..d4f6cf41c0 100644 --- a/codex-rs/core/src/context/world_state/apps_instructions.rs +++ b/codex-rs/core/src/context/world_state/apps_instructions.rs @@ -1,28 +1,133 @@ +//! Reconciles the newest retained Apps fragment with the selected model's wording. +//! The persisted availability bit lets older clients restore their bundled guidance. + use super::PreviousSectionState; use super::SectionTransition; +use super::WorldStateHash; use super::WorldStateSection; use super::WorldStateUpdate; use crate::context::AppsInstructions; +use crate::context::CatalogAppsInstructions; use crate::context::ContextualUserFragment; +use codex_prompts::ResolvedAppsInstructions; +use serde::Deserialize; +use serde::Serialize; -/// Whether generic Apps usage guidance should be visible to the model. -#[derive(Clone, Copy, Debug, Default)] +const REPLACEMENT_NOTICE: &str = + "\nThese Apps usage instructions replace all previously provided Apps usage instructions.\n\n"; +const REMOVAL_NOTICE: &str = "\nThe previously provided Apps usage instructions no longer apply.\n"; + +#[derive(Clone, Debug)] pub(crate) struct AppsInstructionsState { available: bool, + resolved: ResolvedAppsInstructions, +} + +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(untagged)] +pub(crate) enum AppsInstructionsSnapshot { + /// The pre-catalog reader appends its bundled instructions on false. + BundledAvailable(bool), + /// Read existing catalog checkpoints. We still use this form when catalog wording is + /// byte-identical to bundled wording: an old reader can keep it, while a new reader + /// must remember its catalog origin if Apps become unavailable. + Catalog { + instructions: Option, + catalog: bool, + }, +} + +/// The instructions that still apply in the retained history. +enum RetainedGuidance<'a> { + None, + Bundled, + Catalog(&'a str), +} + +impl<'a> RetainedGuidance<'a> { + fn recover( + previous: PreviousSectionState<'_, AppsInstructionsSnapshot>, + retained: Option<&'a str>, + bundled: &'a str, + catalog_selected: bool, + ) -> Self { + use AppsInstructionsSnapshot::BundledAvailable; + use AppsInstructionsSnapshot::Catalog; + + // The World State matcher has already checked the developer role and both markers. + let body = retained.map(str::trim).map(|text| { + let (open, close) = if CatalogAppsInstructions::matches_text(text) { + CatalogAppsInstructions::type_markers() + } else { + AppsInstructions::type_markers() + }; + &text[open.len()..text.len() - close.len()] + }); + let Some(body) = body else { + return match previous { + PreviousSectionState::Absent + | PreviousSectionState::Known(BundledAvailable(false)) => Self::None, + PreviousSectionState::Unknown + | PreviousSectionState::Known(BundledAvailable(true)) => Self::Bundled, + PreviousSectionState::Known(Catalog { + instructions: None, .. + }) => Self::None, + PreviousSectionState::Known(Catalog { + instructions: Some(_), + .. + }) => Self::Bundled, + }; + }; + // Prior catalog checkpoints carried a hash to distinguish literal notice-like content. + let saved_literal = matches!(previous, PreviousSectionState::Known(Catalog { instructions: Some(hash), .. }) + if (body == REMOVAL_NOTICE || body.starts_with(REPLACEMENT_NOTICE)) && *hash == fingerprint(body)); + let wording = if saved_literal { + body + } else if body == REMOVAL_NOTICE { + return Self::None; + } else { + body.strip_prefix(REPLACEMENT_NOTICE).unwrap_or(body) + }; + if wording != bundled { + return Self::Catalog(wording); + } + match previous { + PreviousSectionState::Known(Catalog { catalog: true, .. }) => Self::Catalog(wording), + // Older clients can disable Apps without appending a retirement notice. Re-enable + // their bundled wording normally, but let an explicit catalog choice supersede it. + PreviousSectionState::Known( + BundledAvailable(false) + | Catalog { + instructions: None, + catalog: false, + }, + ) if !catalog_selected => Self::None, + _ => Self::Bundled, + } + } +} + +fn fingerprint(text: &str) -> WorldStateHash { + WorldStateHash::from_json(&serde_json::Value::String(text.to_owned())) } impl AppsInstructionsState { - pub(crate) fn new(available: bool) -> Self { - Self { available } + pub(crate) fn new(available: bool, resolved: ResolvedAppsInstructions) -> Self { + Self { + available, + resolved, + } } } impl WorldStateSection for AppsInstructionsState { const ID: &'static str = "apps_instructions"; - type Snapshot = bool; + type Snapshot = AppsInstructionsSnapshot; + const NEEDS_LATEST_FRAGMENT: bool = true; fn matches_legacy_fragment(role: &str, text: &str) -> bool { - role == "developer" && AppsInstructions::matches_text(text) + role == "developer" + && (AppsInstructions::matches_text(text) || CatalogAppsInstructions::matches_text(text)) } fn has_retained_fragment_matcher() -> bool { @@ -37,18 +142,60 @@ impl WorldStateSection for AppsInstructionsState { &self, previous: PreviousSectionState<'_, Self::Snapshot>, ) -> SectionTransition { - let current = self.available; - if !self.available - || matches!(previous, PreviousSectionState::Known(previous) if *previous) - || matches!(previous, PreviousSectionState::Unknown) - { - return (Some(current), Vec::new()); - } + self.render_retained_diff(previous, None) + } - ( - Some(current), - vec![WorldStateUpdate::fragment(AppsInstructions)], - ) + fn render_retained_diff( + &self, + previous: PreviousSectionState<'_, Self::Snapshot>, + retained: Option<&str>, + ) -> SectionTransition { + let bundled = AppsInstructions::default().0; + let catalog = self.resolved.catalog_override; + let previous = RetainedGuidance::recover(previous, retained, &bundled, catalog); + let instructions = self + .available + .then_some(self.resolved.usage_instructions.as_str()) + .filter(|text| !text.is_empty()); + let is_bundled = instructions == Some(bundled.as_str()); + let snapshot = if catalog && is_bundled { + AppsInstructionsSnapshot::Catalog { + instructions: Some(fingerprint(&bundled)), + catalog: true, + } + } else { + AppsInstructionsSnapshot::BundledAvailable(is_bundled) + }; + let previous_wording = match previous { + RetainedGuidance::None => None, + RetainedGuidance::Bundled => Some(bundled.as_str()), + RetainedGuidance::Catalog(text) => Some(text), + }; + if previous_wording == instructions { + return (Some(snapshot), Vec::new()); + } + let body = match instructions { + // A notice-like literal must have a replacement prefix even on its first appearance, + // so recovery can strip exactly one layer and keep every byte of the supplied text. + Some(text) + if previous_wording.is_some() + || text.starts_with(REPLACEMENT_NOTICE) + || text == REMOVAL_NOTICE => + { + format!("{REPLACEMENT_NOTICE}{text}") + } + Some(text) => text.to_owned(), + None if catalog || matches!(previous, RetainedGuidance::Catalog(_)) => { + REMOVAL_NOTICE.to_owned() + } + None => return (Some(snapshot), Vec::new()), + }; + let update = if is_bundled { + WorldStateUpdate::fragment(AppsInstructions(body)) + } else { + WorldStateUpdate::fragment(CatalogAppsInstructions(body)) + }; + (Some(snapshot), vec![update]) } } diff --git a/codex-rs/core/src/context/world_state/apps_instructions_tests.rs b/codex-rs/core/src/context/world_state/apps_instructions_tests.rs index 301763d7b3..4401bbe251 100644 --- a/codex-rs/core/src/context/world_state/apps_instructions_tests.rs +++ b/codex-rs/core/src/context/world_state/apps_instructions_tests.rs @@ -1,9 +1,14 @@ +#![cfg(test)] + use super::*; -use crate::context::ContextualUserFragment; -use crate::context::world_state::PreviousSectionState; +use crate::context::world_state::WorldState; +use crate::context::world_state::WorldStateSnapshot; use crate::context::world_state::test_support::render_section_cases; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; +use codex_prompts::ResolvedModelMessages; use codex_protocol::models::ResponseItem; use pretty_assertions::assert_eq; +use serde_json::json; #[test] fn snapshots() { @@ -11,8 +16,10 @@ fn snapshots() { use PreviousSectionState::Known; use PreviousSectionState::Unknown; - let unavailable = AppsInstructionsState::new(/*available*/ false); - let available = AppsInstructionsState::new(/*available*/ true); + let bundled = + ResolvedModelMessages::bundled().apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME); + let unavailable = AppsInstructionsState::new(/*available*/ false, bundled.clone()); + let available = AppsInstructionsState::new(/*available*/ true, bundled); insta::assert_snapshot!(render_section_cases(&[ (Absent, Absent), @@ -27,37 +34,411 @@ fn snapshots() { } #[test] -fn legacy_guidance_is_not_injected_again() { - let mut world_state = super::super::WorldState::default(); - world_state.add_section(AppsInstructionsState::new(/*available*/ true)); - let legacy: ResponseItem = ContextualUserFragment::into(AppsInstructions); +fn legacy_guidance_is_reused_or_retired_by_catalog_intent() { + let bundled = AppsInstructions::default().0; + let replacement = format!("{REPLACEMENT_NOTICE}{bundled}"); + let custom = format!("{REPLACEMENT_NOTICE}B"); + let removal = REMOVAL_NOTICE.to_owned(); + for (saved_available, retained, catalog, available, expected) in [ + (None, bundled.as_str(), None, true, None), + (Some(true), bundled.as_str(), None, true, None), + ( + Some(false), + bundled.as_str(), + Some("B"), + false, + Some(removal.as_str()), + ), + ( + Some(false), + bundled.as_str(), + Some(""), + false, + Some(removal.as_str()), + ), + // Older clients can save false while retaining custom instructions from a newer client. + (Some(false), "B", None, true, Some(replacement.as_str())), + (Some(false), "B", None, false, Some(removal.as_str())), + ( + Some(false), + custom.as_str(), + None, + true, + Some(replacement.as_str()), + ), + ( + Some(false), + custom.as_str(), + None, + false, + Some(removal.as_str()), + ), + // Bundled-only false snapshots keep their historical availability-cycle behavior. + ( + Some(false), + bundled.as_str(), + None, + true, + Some(bundled.as_str()), + ), + (Some(false), bundled.as_str(), None, false, None), + ( + Some(false), + replacement.as_str(), + None, + true, + Some(bundled.as_str()), + ), + (Some(false), replacement.as_str(), None, false, None), + ] { + let legacy: ResponseItem = + ContextualUserFragment::into(AppsInstructions(retained.to_string())); + let mut model = codex_models_manager::model_info::model_info_from_slug("gpt-5.5"); + model.model_messages = + Some(serde_json::from_value(json!({"apps": {"usage_instructions": catalog}})).unwrap()); + let mut world_state = WorldState::default(); + world_state.add_section(AppsInstructionsState::new( + available, + ResolvedModelMessages::from_model(&model) + .apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME), + )); + let snapshot: Option = saved_available.map(|available| { + serde_json::from_value(json!({"apps_instructions": available})).unwrap() + }); + assert_eq!( + world_state + .render_history_fragment_diff(snapshot.as_ref(), std::slice::from_ref(&legacy)) + .1 + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect::>(), + expected + .map(|text| { + if text == bundled || text == format!("{REPLACEMENT_NOTICE}{bundled}") { + ContextualUserFragment::into(AppsInstructions(text.to_string())) + } else { + ContextualUserFragment::into(CatalogAppsInstructions(text.to_string())) + } + }) + .into_iter() + .collect::>() + ); + } +} - assert!( - world_state - .render_history_fragment_diff(/*previous*/ None, &[legacy]) - .1 - .is_empty() - ); +#[test] +fn resume_after_an_older_catalog_client_silently_disabled_bundled_guidance() { + let retained = ContextualUserFragment::into(AppsInstructions::default()); + let previous: WorldStateSnapshot = + serde_json::from_value(json!({"apps_instructions": {"catalog": false}})).unwrap(); + let bundled = + ResolvedModelMessages::bundled().apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME); + for (resolved, expected) in [ + (bundled, retained.clone()), + ( + ResolvedAppsInstructions { + usage_instructions: String::new(), + catalog_override: true, + }, + ContextualUserFragment::into(CatalogAppsInstructions(REMOVAL_NOTICE.to_string())), + ), + ] { + let mut world_state = WorldState::default(); + world_state.add_section(AppsInstructionsState::new( + /*available*/ true, resolved, + )); + let (_, updates) = world_state + .render_history_fragment_diff(Some(&previous), std::slice::from_ref(&retained)); + assert_eq!( + updates + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect::>(), + [expected] + ); + } } #[test] fn persisted_guidance_is_restored_only_when_missing_from_history() { - let mut world_state = super::super::WorldState::default(); - world_state.add_section(AppsInstructionsState::new(/*available*/ true)); - let snapshot = world_state.render_full().0; - let retained: ResponseItem = ContextualUserFragment::into(AppsInstructions); - - assert_eq!( - world_state - .render_history_fragment_diff(Some(&snapshot), &[]) - .1 - .len(), - 1 - ); - assert!( - world_state - .render_history_fragment_diff(Some(&snapshot), &[retained]) - .1 - .is_empty() - ); + let retained: ResponseItem = ContextualUserFragment::into(AppsInstructions::default()); + let bundled = + ResolvedModelMessages::bundled().apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME); + for history in [vec![], vec![retained]] { + let mut world_state = WorldState::default(); + world_state.add_section(AppsInstructionsState::new( + /*available*/ true, + bundled.clone(), + )); + let snapshot = world_state.render_full().0; + assert_eq!( + world_state + .render_history_fragment_diff(Some(&snapshot), &history) + .1 + .len(), + usize::from(history.is_empty()) + ); + } +} + +#[test] +fn catalog_guidance_updates_clears_and_returns_to_bundled() { + let bundled = AppsInstructions::default().0; + let replacement = format!("{REPLACEMENT_NOTICE}B"); + let fallback = format!("{REPLACEMENT_NOTICE}{bundled}"); + let removal = REMOVAL_NOTICE.to_owned(); + let mut history = Vec::new(); + let mut previous = None; + for (available, catalog, expected) in [ + (true, Some("A"), Some("A")), + (true, Some("A"), None), + (true, Some("B"), Some(replacement.as_str())), + (true, Some(""), Some(removal.as_str())), + (true, Some(""), None), + (true, Some("A"), Some("A")), + (true, None, Some(fallback.as_str())), + (false, None, None), + (true, None, Some(bundled.as_str())), + (true, Some("B"), Some(replacement.as_str())), + (false, Some("B"), Some(removal.as_str())), + (false, Some("B"), None), + // Selecting catalog intent while unavailable must retire retained bundled guidance. + (true, None, Some(bundled.as_str())), + (false, Some("B"), Some(removal.as_str())), + (false, Some("B"), None), + (true, Some("B"), Some("B")), + (true, None, Some(fallback.as_str())), + (false, Some(""), Some(removal.as_str())), + (true, Some(""), None), + // A prior bundled-only unavailable turn must not hide later catalog intent. + (true, None, Some(bundled.as_str())), + (false, None, None), + (false, Some("B"), Some(removal.as_str())), + (true, Some("B"), Some("B")), + (true, None, Some(fallback.as_str())), + (false, None, None), + (false, Some(""), Some(removal.as_str())), + (true, Some(""), None), + ] { + let mut model = codex_models_manager::model_info::model_info_from_slug("gpt-5.5"); + model.model_messages = + Some(serde_json::from_value(json!({"apps": {"usage_instructions": catalog}})).unwrap()); + let resolved = ResolvedModelMessages::from_model(&model) + .apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME); + let mut world_state = WorldState::default(); + world_state.add_section(AppsInstructionsState::new(available, resolved)); + let (snapshot, fragments) = + world_state.render_history_fragment_diff(previous.as_ref(), &history); + let updates = fragments + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect::>(); + assert_eq!( + updates, + expected + .map(|text| { + if text == bundled || text == format!("{REPLACEMENT_NOTICE}{bundled}") { + ContextualUserFragment::into(AppsInstructions(text.to_string())) + } else { + ContextualUserFragment::into(CatalogAppsInstructions(text.to_string())) + } + }) + .into_iter() + .collect::>() + ); + history.extend(updates); + previous = Some(snapshot); + } +} + +// These are the matching and rendering rules of the released availability-only client. +struct PreCatalogApps; + +impl WorldStateSection for PreCatalogApps { + const ID: &'static str = "apps_instructions"; + type Snapshot = bool; + + fn has_retained_fragment_matcher() -> bool { + true + } + + fn matches_retained_fragment(role: &str, text: &str) -> bool { + role == "developer" && AppsInstructions::matches_text(text) + } + + fn matches_legacy_fragment(role: &str, text: &str) -> bool { + Self::matches_retained_fragment(role, text) + } + + fn render_diff(&self, previous: PreviousSectionState<'_, bool>) -> SectionTransition { + let updates = match previous { + PreviousSectionState::Unknown | PreviousSectionState::Known(true) => Vec::new(), + PreviousSectionState::Absent | PreviousSectionState::Known(false) => { + vec![WorldStateUpdate::fragment(AppsInstructions::default())] + } + }; + (Some(true), updates) + } +} + +#[test] +fn downgrade_restores_bundled_guidance_after_catalog_selection_and_retirement() { + let bundled: ResponseItem = ContextualUserFragment::into(AppsInstructions::default()); + let mut previous = None; + let mut catalog_history = Vec::new(); + let mut older_client = WorldState::default(); + older_client.add_section(PreCatalogApps); + for text in ["custom", ""] { + let mut world_state = WorldState::default(); + world_state.add_section(AppsInstructionsState::new( + /*available*/ true, + ResolvedAppsInstructions { + usage_instructions: text.to_string(), + catalog_override: true, + }, + )); + let (snapshot, updates) = + world_state.render_history_fragment_diff(previous.as_ref(), &catalog_history); + assert_eq!(updates.len(), 1); + catalog_history.extend( + updates + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item), + ); + let saved = serde_json::to_string(&snapshot).unwrap(); + let restored: WorldStateSnapshot = serde_json::from_str(&saved).unwrap(); + let mixed_history = std::iter::once(&bundled).chain(&catalog_history); + for (saved, history) in [ + (Some(&restored), mixed_history.collect::>()), + (None, catalog_history.iter().collect()), + ] { + assert_eq!( + older_client + .render_history_fragment_diff(saved, history) + .1 + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect::>(), + std::slice::from_ref(&bundled) + ); + } + previous = Some(snapshot); + } +} + +#[test] +fn literal_catalog_notices_remain_active_guidance_after_snapshot_loss() { + for text in [ + REMOVAL_NOTICE.to_owned(), + format!("{REPLACEMENT_NOTICE}literal"), + ] { + let mut world_state = WorldState::default(); + world_state.add_section(AppsInstructionsState::new( + /*available*/ true, + ResolvedAppsInstructions { + usage_instructions: text.clone(), + catalog_override: true, + }, + )); + let (_, first) = world_state.render_full_fragments(); + let history: Vec<_> = first + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect(); + assert_eq!( + history, + [ContextualUserFragment::into(CatalogAppsInstructions( + format!("{REPLACEMENT_NOTICE}{text}") + ))] + ); + assert!( + world_state + .render_history_fragment_diff(/*previous*/ None, &history) + .1 + .is_empty() + ); + } +} + +#[test] +fn retained_catalog_guidance_recovers_legacy_snapshots_and_repairs_lost_updates() { + let mut history = Vec::new(); + let mut previous = None; + let catalog = |text| ContextualUserFragment::into(CatalogAppsInstructions(text)); + let replacement = |text: &str| format!("{REPLACEMENT_NOTICE}{text}"); + let removal = catalog(REMOVAL_NOTICE.to_owned()); + let two_byte_limit = "é".repeat(400); + let four_byte_limit = "\u{10ffff}".repeat(200); + let oversized = "é".repeat(500); + for (available, text, expected) in [ + (true, "A", catalog("A".to_string())), + (true, "B", catalog(replacement("B"))), + (false, "B", removal.clone()), + (true, "B\r\n", catalog("B\r\n".to_string())), + (true, "B\n", catalog(replacement("B\n"))), + ( + true, + two_byte_limit.as_str(), + catalog(replacement(&two_byte_limit)), + ), + ( + true, + four_byte_limit.as_str(), + catalog(replacement(&four_byte_limit)), + ), + ( + true, + oversized.as_str(), + ContextualUserFragment::into(AppsInstructions(replacement( + &AppsInstructions::default().0, + ))), + ), + (true, "", removal), + ] { + let mut model = codex_models_manager::model_info::model_info_from_slug("gpt-5.5"); + model.model_messages = + Some(serde_json::from_value(json!({"apps": {"usage_instructions": text}})).unwrap()); + let resolved = ResolvedModelMessages::from_model(&model) + .apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME); + let mut world_state = WorldState::default(); + world_state.add_section(AppsInstructionsState::new(available, resolved)); + let (snapshot, fragments) = + world_state.render_history_fragment_diff(previous.as_ref(), &history); + for fragment in &fragments { + assert!(fragment.render().into_text().unwrap().len() <= 1_000); + } + let updates = fragments + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect::>(); + assert_eq!(updates, [expected]); + for saved in [ + None, + Some(json!({"apps_instructions": true})), + Some(serde_json::to_value(&snapshot).unwrap()), + ] { + let saved: Option = + saved.map(|value| serde_json::from_value(value).unwrap()); + assert!( + world_state + .render_history_fragment_diff(saved.as_ref(), &updates) + .1 + .is_empty() + ); + } + if !history.is_empty() { + // The newest update was compacted away; an older marker must not suppress repair. + assert_eq!( + world_state + .render_history_fragment_diff(Some(&snapshot), &history) + .1 + .into_iter() + .map(ContextualUserFragment::into_boxed_response_item) + .collect::>(), + updates + ); + } + history.extend(updates); + previous = Some(snapshot); + } } diff --git a/codex-rs/core/src/context/world_state/mod.rs b/codex-rs/core/src/context/world_state/mod.rs index 7b3dbe3908..8781e37a50 100644 --- a/codex-rs/core/src/context/world_state/mod.rs +++ b/codex-rs/core/src/context/world_state/mod.rs @@ -171,7 +171,15 @@ trait ErasedWorldStateSection: Send + Sync { fn matches_retained_fragment(&self, role: &str, text: &str) -> bool; - fn render_diff(&self, previous: PreviousSectionState<'_, Value>) -> SectionTransition; + fn needs_latest_fragment(&self) -> bool { + false + } + + fn render_diff( + &self, + previous: PreviousSectionState<'_, Value>, + retained: Option<&str>, + ) -> SectionTransition; } fn section_snapshot(section: &S, snapshot: S::Snapshot) -> Option { @@ -213,7 +221,15 @@ impl ErasedWorldStateSection for S { S::matches_retained_fragment(role, text) } - fn render_diff(&self, previous: PreviousSectionState<'_, Value>) -> SectionTransition { + fn needs_latest_fragment(&self) -> bool { + S::NEEDS_LATEST_FRAGMENT + } + + fn render_diff( + &self, + previous: PreviousSectionState<'_, Value>, + retained: Option<&str>, + ) -> SectionTransition { let typed_snapshot; let previous = match previous { PreviousSectionState::Known(previous) => { @@ -236,7 +252,7 @@ impl ErasedWorldStateSection for S { PreviousSectionState::Absent => PreviousSectionState::Absent, PreviousSectionState::Unknown => PreviousSectionState::Unknown, }; - let (snapshot, updates) = WorldStateSection::render_diff(self, previous); + let (snapshot, updates) = self.render_retained_diff(previous, retained); ( snapshot.map(|snapshot| section_snapshot(self, snapshot).unwrap_or(Value::Null)), updates, @@ -263,7 +279,11 @@ impl ErasedWorldStateSection for ExtensionWorldStateSection { self.0.matches_retained_fragment(role, text) } - fn render_diff(&self, previous: PreviousSectionState<'_, Value>) -> SectionTransition { + fn render_diff( + &self, + previous: PreviousSectionState<'_, Value>, + _retained: Option<&str>, + ) -> SectionTransition { let previous = match previous { PreviousSectionState::Absent => PreviousWorldStateSection::Absent, PreviousSectionState::Unknown => PreviousWorldStateSection::Unknown, @@ -350,6 +370,17 @@ pub(crate) trait WorldStateSection: Send + Sync + 'static { const ID: &'static str; type Snapshot: DeserializeOwned + Serialize; + /// Whether history recovery needs the most recent match instead of any match. + const NEEDS_LATEST_FRAGMENT: bool = false; + + fn render_retained_diff( + &self, + previous: PreviousSectionState<'_, Self::Snapshot>, + _retained: Option<&str>, + ) -> SectionTransition { + self.render_diff(previous) + } + /// Whether the section contributes comparison state to persisted rollouts. fn should_persist(&self) -> bool { true @@ -620,7 +651,7 @@ impl WorldState { pub(crate) fn render_full(&self) -> (WorldStateSnapshot, Vec) { self.render_with(RenderScope::FullContext, /*stored*/ None, |_, _| { - PreviousSectionState::Absent + (PreviousSectionState::Absent, None) }) } @@ -628,22 +659,48 @@ impl WorldState { pub(crate) fn render_history_diff<'a>( &self, previous: Option<&WorldStateSnapshot>, - items: impl IntoIterator + Clone, + items: impl IntoIterator + Clone, ) -> (WorldStateSnapshot, Vec) { self.render_with(RenderScope::HistoryDiff, previous, |id, section| { - if let Some(previous) = previous.and_then(|previous| previous.section(id)) { - if section.has_retained_fragment_matcher() - && !has_retained_fragment(items.clone(), section) - { - PreviousSectionState::Absent - } else { - PreviousSectionState::Known(previous) - } - } else if has_legacy_fragment(items.clone(), section) { - PreviousSectionState::Unknown - } else { - PreviousSectionState::Absent + let stored = previous.and_then(|previous| previous.section(id)); + if let Some(stored) = stored + && !section.has_retained_fragment_matcher() + { + return (PreviousSectionState::Known(stored), None); } + let matches = |role: &str, text: &str| { + if stored.is_some() { + section.matches_retained_fragment(role, text) + } else { + section.matches_legacy_fragment(role, text) + } + }; + let mut fragments = items + .clone() + .into_iter() + .filter_map(|item| match item { + ResponseItem::Message { role, content, .. } => Some((role, content)), + _ => None, + }) + .flat_map(|(role, content)| { + content.iter().filter_map(move |item| match item { + ContentItem::InputText { text } if matches(role, text) => { + Some(text.as_str()) + } + _ => None, + }) + }); + let retained = if section.needs_latest_fragment() { + fragments.next_back() + } else { + fragments.next() + }; + let state = match (stored, retained) { + (_, None) => PreviousSectionState::Absent, + (None, Some(_)) => PreviousSectionState::Unknown, + (Some(stored), Some(_)) => PreviousSectionState::Known(stored), + }; + (state, retained) }) } @@ -651,7 +708,10 @@ impl WorldState { &self, scope: RenderScope, stored: Option<&WorldStateSnapshot>, - mut previous: impl FnMut(&str, &dyn ErasedWorldStateSection) -> PreviousSectionState<'a, Value>, + mut previous: impl FnMut( + &str, + &dyn ErasedWorldStateSection, + ) -> (PreviousSectionState<'a, Value>, Option<&'a str>), ) -> (WorldStateSnapshot, Vec) { let mut snapshot = WorldStateSnapshot::default(); let mut updates = Vec::new(); @@ -659,8 +719,8 @@ impl WorldState { if matches!(scope, RenderScope::FullContext) && !section.include_in_full_context() { continue; } - let prior = previous(id, section.as_ref()); - let (section_snapshot, section_updates) = section.render_diff(prior); + let (prior, retained) = previous(id, section.as_ref()); + let (section_snapshot, section_updates) = section.render_diff(prior, retained); // A skipped snapshot retains stored state even if history required a fresh render. let section_snapshot = section_snapshot .or_else(|| stored.and_then(|snapshot| snapshot.section(id)).cloned()); @@ -675,44 +735,6 @@ impl WorldState { } } -fn has_retained_fragment<'a>( - items: impl IntoIterator, - section: &dyn ErasedWorldStateSection, -) -> bool { - items.into_iter().any(|item| { - matches!( - item, - ResponseItem::Message { role, content, .. } - if content.iter().any(|content| { - matches!( - content, - ContentItem::InputText { text } - if section.matches_retained_fragment(role, text) - ) - }) - ) - }) -} - -fn has_legacy_fragment<'a>( - items: impl IntoIterator, - section: &dyn ErasedWorldStateSection, -) -> bool { - items.into_iter().any(|item| { - matches!( - item, - ResponseItem::Message { role, content, .. } - if content.iter().any(|content| { - matches!( - content, - ContentItem::InputText { text } - if section.matches_legacy_fragment(role, text) - ) - }) - ) - }) -} - fn remove_null_object_fields(value: &mut Value) { // RFC 7386 reserves object-valued nulls for deletion, but arrays are replaced whole. match value { diff --git a/codex-rs/core/src/context/world_state/test_support.rs b/codex-rs/core/src/context/world_state/test_support.rs index 46f55267af..8d6ca30f88 100644 --- a/codex-rs/core/src/context/world_state/test_support.rs +++ b/codex-rs/core/src/context/world_state/test_support.rs @@ -81,7 +81,7 @@ fn render_diff( PreviousSectionState::Known(&previous_snapshot) } }; - ErasedWorldStateSection::render_diff(*after, previous).1 + ErasedWorldStateSection::render_diff(*after, previous, /*retained*/ None).1 } fn render_snapshot(section: &S) -> String { @@ -109,9 +109,13 @@ fn sort_json(value: serde_json::Value) -> serde_json::Value { } fn snapshot_value(section: &S) -> serde_json::Value { - ErasedWorldStateSection::render_diff(section, PreviousSectionState::Absent) - .0 - .expect("world-state section snapshot should serialize to a non-null value") + ErasedWorldStateSection::render_diff( + section, + PreviousSectionState::Absent, + /*retained*/ None, + ) + .0 + .expect("world-state section snapshot should serialize to a non-null value") } /// Keeps text-only section assertions explicit when a section gains item output. @@ -165,7 +169,7 @@ impl WorldState { pub(crate) fn render_history_fragment_diff<'a>( &self, previous: Option<&WorldStateSnapshot>, - items: impl IntoIterator + Clone, + items: impl IntoIterator + Clone, ) -> (WorldStateSnapshot, Vec>) { let (snapshot, updates) = self.render_history_diff(previous, items); (snapshot, expect_fragments(updates)) diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index 62bd597a8e..1bb1c18ea2 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -2277,6 +2277,9 @@ async fn drop_last_n_user_turns_trims_context_updates_above_rolled_back_turn() { developer_msg(&format!( "{APPS_INSTRUCTIONS_OPEN_TAG}\nROLLED_BACK_APPS_INSTRUCTIONS" )), + developer_msg( + "Retired Apps guidance", + ), developer_msg(&format!( "{PLUGINS_INSTRUCTIONS_OPEN_TAG}\nROLLED_BACK_PLUGIN_INSTRUCTIONS" )), diff --git a/codex-rs/core/src/event_mapping.rs b/codex-rs/core/src/event_mapping.rs index b95badc68d..75a1191272 100644 --- a/codex-rs/core/src/event_mapping.rs +++ b/codex-rs/core/src/event_mapping.rs @@ -14,6 +14,7 @@ use codex_protocol::models::WebSearchAction; use codex_protocol::models::is_closing_media_label; use codex_protocol::models::is_opening_media_label; use codex_protocol::protocol::APPS_INSTRUCTIONS_OPEN_TAG; +use codex_protocol::protocol::CATALOG_APPS_INSTRUCTIONS_OPEN_TAG; use codex_protocol::protocol::COLLABORATION_MODE_OPEN_TAG; use codex_protocol::protocol::CONTEXT_WINDOW_GUIDANCE_OPEN_TAG; use codex_protocol::protocol::CONTEXT_WINDOW_OPEN_TAG; @@ -39,6 +40,7 @@ const CONTEXTUAL_DEVELOPER_PREFIXES: &[&str] = &[ "", "", APPS_INSTRUCTIONS_OPEN_TAG, + CATALOG_APPS_INSTRUCTIONS_OPEN_TAG, COLLABORATION_MODE_OPEN_TAG, "", MULTI_AGENT_MODE_OPEN_TAG, diff --git a/codex-rs/core/src/session/world_state.rs b/codex-rs/core/src/session/world_state.rs index 72944bcb7b..fa02844b5e 100644 --- a/codex-rs/core/src/session/world_state.rs +++ b/codex-rs/core/src/session/world_state.rs @@ -32,6 +32,7 @@ use codex_connectors::AppToolPolicyEvaluator; use codex_extension_api::WorldStateContributionInput; use codex_features::Feature; use codex_file_system::FileSystemSandboxContext; +use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_prompts::ApprovalPromptContext; use codex_prompts::ResolvedModelMessages; use codex_prompts::render_model_instructions; @@ -372,8 +373,18 @@ impl Session { }; let apps_usage_instructions_available = apps_available && model_info.include_apps_usage_instructions; + let apps_messages = if turn_context + .config + .features + .enabled(Feature::CatalogAppsInstructions) + { + model_messages + } else { + ResolvedModelMessages::bundled() + }; world_state.add_section(AppsInstructionsState::new( apps_usage_instructions_available, + apps_messages.apps_usage_instructions(CODEX_APPS_MCP_SERVER_NAME), )); let plugins_usage_instructions_available = step_context.mcp.plugins_available() && model_info.include_plugin_usage_instructions; diff --git a/codex-rs/core/tests/suite/client.rs b/codex-rs/core/tests/suite/client.rs index 7420519311..02a2060b00 100644 --- a/codex-rs/core/tests/suite/client.rs +++ b/codex-rs/core/tests/suite/client.rs @@ -2188,8 +2188,33 @@ async fn includes_user_instructions_message_in_request() { ); } +const CATALOG_APPS_GUIDANCE: &str = "Distinctive catalog Apps guidance."; + +fn assert_no_apps_guidance(request: &ResponsesRequest) { + for text in [ + "", + "", + CATALOG_APPS_GUIDANCE, + ] { + assert!( + !request.body_contains_text(text), + "unexpected Apps guidance: {text:?}" + ); + } +} + +#[test_case::test_case(false, true, true, true; "catalog_missing")] +#[test_case::test_case(true, false, true, true; "catalog_feature_off_by_default")] +#[test_case::test_case(true, true, true, true; "catalog_override")] +#[test_case::test_case(true, true, false, true; "model_excludes")] +#[test_case::test_case(true, true, true, false; "apps_feature_off")] #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() { +async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth( + catalog_override: bool, + enable_catalog: bool, + model_includes: bool, + apps_enabled: bool, +) { skip_if_no_network!(); let server = MockServer::start().await; let apps_server = AppsTestServer::mount(&server) @@ -2208,12 +2233,27 @@ async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() { .with_config(move |config| { config .features - .enable(Feature::Apps) + .set_enabled(Feature::Apps, apps_enabled) .expect("test config should allow feature update"); + if enable_catalog { + config + .features + .enable(Feature::CatalogAppsInstructions) + .expect("enable catalog Apps instructions"); + } config.chatgpt_base_url = apps_base_url; + }) + .with_model_info_override("gpt-5.5", move |model| { + model.include_apps_usage_instructions = model_includes; + model.model_messages = catalog_override.then(|| { + serde_json::from_value( + json!({"apps": {"usage_instructions": CATALOG_APPS_GUIDANCE}}), + ) + .expect("catalog Apps messages") + }); }); let codex = builder - .build(&server) + .build_with_auto_env(&server) .await .expect("create new conversation") .codex; @@ -2224,19 +2264,32 @@ async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() { text_elements: Vec::new(), }])) .await - .unwrap(); + .expect("start Apps guidance turn"); wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; let request = resp_mock.single_request(); - let apps_snippet = - "Apps (Connectors) can be explicitly triggered in user messages in the format"; - - assert!( - message_input_text_contains(&request, "developer", apps_snippet), - "expected apps guidance in a developer message, got {:?}", - request.body_json()["input"] + let use_catalog = catalog_override && enable_catalog; + assert_eq!( + request.body_contains_text(CATALOG_APPS_GUIDANCE), + use_catalog && model_includes && apps_enabled ); + let apps_snippet = if use_catalog { + CATALOG_APPS_GUIDANCE + } else { + "Apps (Connectors) can be explicitly triggered in user messages in the format" + }; + + assert_eq!( + message_input_text_contains(&request, "developer", apps_snippet), + model_includes && apps_enabled + ); + if !model_includes || !apps_enabled { + assert_no_apps_guidance(&request); + } + if !apps_enabled { + assert!(!request.body_contains_text("mcp__codex_apps")); + } assert!( !message_input_text_contains(&request, "user", apps_snippet), @@ -2245,6 +2298,121 @@ async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() { ); } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn resume_without_apps_checkpoint_restores_bundled_guidance_once() -> anyhow::Result<()> { + skip_if_no_network!(Ok(())); + let server = MockServer::start().await; + let apps = AppsTestServer::mount(&server).await?; + let builder = || { + let base_url = apps.chatgpt_base_url.clone(); + let mut builder = test_codex() + .with_auth(create_dummy_codex_auth()) + .with_config(move |config| { + config.chatgpt_base_url = base_url; + config.features.enable(Feature::Apps).unwrap(); + }); + for (model, wording) in [ + ("apps-bundled", None), + ("apps-custom", Some(CATALOG_APPS_GUIDANCE)), + ("apps-clear", Some("")), + ] { + builder = builder.with_model_info_override(model, move |info| { + info.include_apps_usage_instructions = true; + info.model_messages = Some( + serde_json::from_value(json!({"apps": {"usage_instructions": wording}})) + .unwrap(), + ); + }); + } + builder + }; + let writer = builder() + .with_model("apps-bundled") + .with_config(|config| { + config + .features + .enable(Feature::CatalogAppsInstructions) + .unwrap(); + }) + .build(&server) + .await?; + let mock = mount_sse_sequence( + &server, + (0..5) + .map(|i| sse(vec![ev_completed(&format!("apps-{i}"))])) + .collect(), + ) + .await; + writer.submit_text_turn("Use bundled guidance").await?; + for model in ["apps-custom", "apps-clear"] { + core_test_support::submit_thread_settings( + &writer.codex, + ThreadSettingsOverrides { + model: Some(model.to_string()), + ..Default::default() + }, + ) + .await?; + writer.submit_text_turn(model).await?; + } + writer.codex.shutdown_and_wait().await?; + + // Preserve the recorded conversation while simulating loss of the Apps checkpoint. + let path = writer.codex.rollout_path().expect("rollout path"); + let mut rollout = String::new(); + let mut removed_checkpoint = false; + for line in std::fs::read_to_string(&path)?.lines() { + let mut record = codex_rollout::parse_rollout_line(line)?; + if let RolloutItem::WorldState(state) = &mut record.item + && state.state.remove("apps_instructions").is_some() + { + removed_checkpoint = true; + rollout.push_str(&serde_json::to_string(&record)?); + } else { + rollout.push_str(line); + } + rollout.push('\n'); + } + assert!(removed_checkpoint); + std::fs::write(&path, rollout)?; + + // Resume on the updated reader with catalog wording still off. + let reader = builder() + .with_model("apps-clear") + .resume(&server, Arc::clone(&writer.home), path) + .await?; + reader + .submit_text_turn("Resume with bundled guidance") + .await?; + reader.submit_text_turn("Continue").await?; + let guidance = |request: &ResponsesRequest| { + request + .message_input_texts("developer") + .into_iter() + .filter(|text| { + text.starts_with("") + || text.starts_with("") + }) + .collect::>() + }; + let requests = mock.requests(); + assert_eq!(requests.len(), 5); + let [bundled, replacement, retirement]: [String; 3] = guidance(&requests[2]) + .try_into() + .expect("bundled, replaced, then retired"); + assert!(bundled.starts_with("")); + assert!(replacement.contains(CATALOG_APPS_GUIDANCE)); + assert!( + retirement.contains("The previously provided Apps usage instructions no longer apply.") + ); + let expected = vec![bundled.clone(), replacement, retirement, bundled]; + assert_eq!(guidance(&requests[0]), expected[..1]); + assert_eq!(guidance(&requests[1]), expected[..2]); + assert_eq!(guidance(&requests[3]), expected); + assert_eq!(guidance(&requests[4]), expected); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn omits_apps_guidance_for_api_key_auth_even_when_feature_enabled() { skip_if_no_network!(); @@ -2267,10 +2435,22 @@ async fn omits_apps_guidance_for_api_key_auth_even_when_feature_enabled() { .features .enable(Feature::Apps) .expect("test config should allow feature update"); + config + .features + .enable(Feature::CatalogAppsInstructions) + .expect("enable catalog Apps instructions"); config.chatgpt_base_url = apps_base_url; + }) + .with_model_info_override("gpt-5.5", |model| { + model.model_messages = Some( + serde_json::from_value( + json!({"apps": {"usage_instructions": CATALOG_APPS_GUIDANCE}}), + ) + .expect("catalog Apps messages"), + ); }); let codex = builder - .build(&server) + .build_with_auto_env(&server) .await .expect("create new conversation") .codex; @@ -2281,19 +2461,16 @@ async fn omits_apps_guidance_for_api_key_auth_even_when_feature_enabled() { text_elements: Vec::new(), }])) .await - .unwrap(); + .expect("start API-key Apps guidance turn"); wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; let request = resp_mock.single_request(); - let apps_snippet = - "Apps (Connectors) can be explicitly triggered in user messages in the format"; - + assert_no_apps_guidance(&request); assert!( - !message_input_text_contains(&request, "developer", apps_snippet) - && !message_input_text_contains(&request, "user", apps_snippet), - "did not expect apps guidance for API key auth, got {:?}", - request.body_json()["input"] + !request.body_contains_text("mcp__codex_apps"), + "did not expect Apps tools for API key auth, got {:?}", + request.body_json()["tools"] ); } @@ -2319,11 +2496,23 @@ async fn omits_apps_guidance_when_configured_off() { .features .enable(Feature::Apps) .expect("test config should allow feature update"); + config + .features + .enable(Feature::CatalogAppsInstructions) + .expect("enable catalog Apps instructions"); config.chatgpt_base_url = apps_base_url; config.include_apps_instructions = false; + }) + .with_model_info_override("gpt-5.5", |model| { + model.model_messages = Some( + serde_json::from_value( + json!({"apps": {"usage_instructions": CATALOG_APPS_GUIDANCE}}), + ) + .unwrap(), + ); }); let codex = builder - .build(&server) + .build_with_auto_env(&server) .await .expect("create new conversation") .codex; @@ -2339,11 +2528,7 @@ async fn omits_apps_guidance_when_configured_off() { wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; let request = resp_mock.single_request(); - assert!( - !message_input_text_contains(&request, "developer", ""), - "did not expect apps instructions when include_apps_instructions = false, got {:?}", - request.body_json()["input"] - ); + assert_no_apps_guidance(&request); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -2390,11 +2575,23 @@ async fn omits_apps_guidance_when_orchestrator_mcp_is_disabled() { .features .enable(Feature::Apps) .expect("test config should allow feature update"); + config + .features + .enable(Feature::CatalogAppsInstructions) + .expect("enable catalog Apps instructions"); config.chatgpt_base_url = apps_base_url; config.orchestrator_mcp_enabled = false; + }) + .with_model_info_override("gpt-5.5", |model| { + model.model_messages = Some( + serde_json::from_value( + json!({"apps": {"usage_instructions": CATALOG_APPS_GUIDANCE}}), + ) + .expect("catalog Apps messages"), + ); }); let codex = builder - .build(&server) + .build_with_auto_env(&server) .await .expect("create new conversation") .codex; @@ -2405,18 +2602,16 @@ async fn omits_apps_guidance_when_orchestrator_mcp_is_disabled() { text_elements: Vec::new(), }])) .await - .unwrap(); + .expect("start disabled-orchestrator Apps guidance turn"); wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; let requests = resp_mock.requests(); assert_eq!(requests.len(), 3); + for request in &requests { + assert_no_apps_guidance(request); + } let request = &requests[0]; - assert!( - !message_input_text_contains(request, "developer", ""), - "did not expect apps instructions when orchestrator MCP is disabled, got {:?}", - request.body_json()["input"] - ); assert!( !request.body_contains_text("mcp__codex_apps"), "did not expect codex_apps MCP tools when orchestrator MCP is disabled, got {:?}", diff --git a/codex-rs/core/tests/suite/scenarios.rs b/codex-rs/core/tests/suite/scenarios.rs index 945ae1f2e9..2fb2dc43de 100644 --- a/codex-rs/core/tests/suite/scenarios.rs +++ b/codex-rs/core/tests/suite/scenarios.rs @@ -144,6 +144,9 @@ mod catalog_settings; #[path = "scenarios_catalog_approval_instructions_tests.rs"] mod catalog_approval_instructions; +#[path = "scenarios_catalog_apps_instructions_tests.rs"] +mod catalog_apps_instructions; + #[path = "scenarios_managed_human_approval_tests.rs"] mod managed_human_approval; diff --git a/codex-rs/core/tests/suite/scenarios_catalog_apps_instructions_tests.rs b/codex-rs/core/tests/suite/scenarios_catalog_apps_instructions_tests.rs new file mode 100644 index 0000000000..271215f23b --- /dev/null +++ b/codex-rs/core/tests/suite/scenarios_catalog_apps_instructions_tests.rs @@ -0,0 +1,172 @@ +#![cfg(test)] + +//! Complete request history for catalog Apps wording across model switches and resumes. + +use super::*; +use codex_protocol::protocol::ThreadSettingsOverrides; +use core_test_support::apps_test_server::AppsTestServer; +use core_test_support::submit_thread_settings; +use pretty_assertions::assert_eq; + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn catalog_apps_instructions_follow_selected_model() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let apps = AppsTestServer::mount_searchable(&server).await?; + let configure = move |config: &mut Config| { + configure_scenario_catalog(config); + config.workspace_roots = vec![config.cwd.clone()]; + config.chatgpt_base_url = apps.chatgpt_base_url.clone(); + for feature in [Feature::Apps, Feature::CatalogAppsInstructions] { + config.features.enable(feature).unwrap(); + } + let catalog = config.model_catalog.as_mut().unwrap(); + let base = catalog + .models + .iter() + .find(|model| model.slug == "gpt-5.5") + .unwrap() + .clone(); + for (slug, wording, include_guidance) in [ + ("apps-bundled", None, true), + ("apps-no-guidance", Some("Calendar guidance B."), false), + ( + "apps-a", + Some(" Calendar guidance A: {{literal}}.\n"), + true, + ), + ("apps-b", Some("Calendar guidance B."), true), + ("apps-clear", Some(""), true), + ] { + let mut model = base.clone(); + model.slug = slug.to_string(); + model.include_apps_usage_instructions = include_guidance; + model.model_messages = Some( + serde_json::from_value(json!({"apps": {"usage_instructions": wording}})).unwrap(), + ); + catalog.models.push(model); + } + }; + let mut builder = test_codex() + .with_model("apps-bundled") + .with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing()) + .with_config(configure.clone()); + let test = builder.build_with_auto_env(&server).await?; + let mock = mount_sse_sequence( + &server, + (0..8) + .map(|index| sse(vec![ev_completed(&format!("apps-{index}"))])) + .collect(), + ) + .await; + + test.submit_text_turn("Check with bundled guidance.") + .await?; + // Selecting catalog wording cannot override the receiving model's availability gate. + for (model, prompt) in [ + ("apps-no-guidance", "Check while guidance is unavailable."), + ("apps-a", "Check the calendar."), + ("apps-b", "Check again with the new model."), + ] { + submit_thread_settings( + &test.codex, + ThreadSettingsOverrides { + model: Some(model.to_string()), + ..Default::default() + }, + ) + .await?; + test.submit_text_turn(prompt).await?; + } + builder = builder.with_config(configure.clone()).with_model("apps-b"); + let test = builder.restart_with_auto_env(&server, &test).await?; + test.submit_text_turn("Check after restarting with the same guidance.") + .await?; + submit_thread_settings( + &test.codex, + ThreadSettingsOverrides { + model: Some("apps-clear".to_string()), + ..Default::default() + }, + ) + .await?; + test.submit_text_turn("Clear the guidance.").await?; + test.submit_text_turn("Continue with cleared guidance.") + .await?; + builder = builder.with_config(configure).with_model("apps-clear"); + let resumed = builder.restart_with_auto_env(&server, &test).await?; + resumed + .submit_text_turn("Check after restarting with cleared guidance.") + .await?; + + let requests = mock.requests(); + assert_eq!(requests.len(), 8); + assert_eq!( + requests + .iter() + .map(|request| request.body_json()["model"].clone()) + .collect::>(), + vec![ + json!("apps-bundled"), + json!("apps-no-guidance"), + json!("apps-a"), + json!("apps-b"), + json!("apps-b"), + json!("apps-clear"), + json!("apps-clear"), + json!("apps-clear"), + ] + ); + let guidance = requests + .iter() + .map(|request| { + request + .message_input_texts("developer") + .into_iter() + .filter(|text| { + text.starts_with("") + || text.starts_with("") + }) + .collect::>() + }) + .collect::>(); + assert_eq!( + guidance.iter().map(Vec::len).collect::>(), + vec![1, 2, 3, 4, 4, 5, 5, 5] + ); + assert!(guidance[0][0].starts_with("")); + assert!(guidance[2][2].contains(" Calendar guidance A: {{literal}}.\n")); + assert!(guidance[3][3].contains("Calendar guidance B.")); + assert!(guidance[3][3].contains("replace all previously provided Apps usage instructions")); + assert!( + guidance[1][1].contains("The previously provided Apps usage instructions no longer apply.") + ); + assert_eq!(guidance[1][1], guidance[5][4]); + for pair in guidance.windows(2) { + assert!( + pair[1].starts_with(&pair[0]), + "history must stay append-only" + ); + } + assert_eq!( + guidance[4], guidance[3], + "custom resume must not repeat guidance" + ); + assert_eq!( + guidance[6], guidance[5], + "clearing twice must not repeat a notice" + ); + assert_eq!( + guidance[7], guidance[6], + "cleared resume must not repeat a notice" + ); + insta::assert_snapshot!( + "catalog_apps_instructions_follow_selected_model", + context_snapshot::format_request_history_snapshot( + "Apps wording follows the selected model, retires bundled guidance when an unavailable model selects an override, preserves literal bytes, and survives both custom and cleared restarts without duplication.", + &requests, + &ContextSnapshotOptions::default().include_request_settings(), + ) + ); + Ok(()) +} diff --git a/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_apps_instructions__catalog_apps_instructions_follow_selected_model.snap b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_apps_instructions__catalog_apps_instructions_follow_selected_model.snap new file mode 100644 index 0000000000..2bafdd32fd --- /dev/null +++ b/codex-rs/core/tests/suite/snapshots/all__suite__scenarios__catalog_apps_instructions__catalog_apps_instructions_follow_selected_model.snap @@ -0,0 +1,344 @@ +--- +source: core/tests/suite/scenarios_catalog_apps_instructions_tests.rs +assertion_line: 144 +expression: "context_snapshot::format_request_history_snapshot(\"Apps wording follows the selected model, retires bundled guidance when an unavailable model selects an override, preserves literal bytes, and survives both custom and cleared restarts without duplication.\",\n&requests, &ContextSnapshotOptions::default().include_request_settings(),)" +--- +Scenario: Apps wording follows the selected model, retires bundled guidance when an unavailable model selects an override, preserves literal bytes, and survives both custom and cleared restarts without duplication. + +## Window 1 +Settings: + include: ["reasoning.encrypted_content"] + model: "apps-bundled" + parallel_tool_calls: true + prompt_cache_key: "" + reasoning: {"effort":"medium"} + store: false + stream: true + text: {"verbosity":"low"} + tool_choice: "auto" + tools (10; hash=9f8074d045f3bd93): + - function/exec_command: Runs a command in a PTY, returning output or a session ID for ongoing interaction.; args=[cmd, justification, login, max_output_tokens, prefix_rule, sandbox_permissions, shell, tty, workdir, yield_time_ms]; hash=c04108e51a8548c5 + - function/write_stdin: Writes characters to an existing unified exec session and returns recent output.; args=[chars, max_output_tokens, session_id, yield_time_ms]; hash=7fb62d8a3e2df4e2 + - function/list_mcp_resources: Lists resources provided by MCP servers. Resources allow servers to share data that provides cont...; args=[cursor, server]; hash=6792edaff13561bd + - function/list_mcp_resource_templates: Lists resource templates provided by MCP servers. Parameterized resource templates allow servers ...; args=[cursor, server]; hash=60f17c90ef7a4ebb + - function/read_mcp_resource: Read a specific resource from an MCP server given the server name and resource URI.; args=[server, uri]; hash=24246896b39008ae + - function/request_user_input: Request user input for one to three short questions and wait for the response. This tool is only ...; args=[questions]; hash=64a729b25c424fc6 + - custom/apply_patch: The `apply_patch` tool can be used to edit files. This is a FREEFORM tool, so do not wrap the pat...; hash=dea9f12b0111a4ba + - function/view_image: View a local image file from the filesystem when visual inspection is needed. Use this for images...; args=[path]; hash=1908de9c0be7118a + - tool_search: # Tool discovery Searches over deferred tool metadata with BM25 and exposes matching tools for t...; args=[limit, query]; hash=b573ed69c0b0e6c7 + - web_search; hash=f103e327c6870a46 +-- request 1 (turn) -- +00:message/developer[3]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Escalation Requests + + Commands are run outside the sandbox if they are approved by the user, or match an existing rule that al...hell control operators, including but not limited to: [hash=d24ebf5fa4e4b9bb] + + - Pipes: | + - Logical operators: &&, || + + NEVER provide a prefix_rule if your command uses a heredoc or herestring. + + ### Examples + Good examples of prefixes: + - ["npm", "run", "dev"] + - ["gh", "pr", "check"] + - ["cargo", "test"] + + [02] + ## Apps (Connectors) + Apps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{connect...long as the context suggests usage of available apps. [hash=8c2ddeb1adaf185d] + An app is equivalent to a set of MCP tools within the `codex_apps` MCP. + An installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool...re searchable by `tools_search` will be listed by it. [hash=4f9c0d35da3968a5] + Do not additionally call list_mcp_resources or list_mcp_resource_templates for apps. + + [03] + Deferred tool namespaces: + - mcp__codex_apps__calendar: Plan events and manage your calendar. + - multi_agent_v1: Tools for spawning and managing sub-agents. + +01:message/user: + + + + + + [hash=e8c1de60307d0a3d] + +02:message/user: + Check with bundled guidance. + +## Window 2 (after request 1) +Settings: relative to window 1 + model: "apps-no-guidance" +-- request 2 (turn) -- +00:message/developer[3]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Escalation Requests + + Commands are run outside the sandbox if they are approved by the user, or match an existing rule that al...hell control operators, including but not limited to: [hash=d24ebf5fa4e4b9bb] + + - Pipes: | + - Logical operators: &&, || + + NEVER provide a prefix_rule if your command uses a heredoc or herestring. + + ### Examples + Good examples of prefixes: + - ["npm", "run", "dev"] + - ["gh", "pr", "check"] + - ["cargo", "test"] + + [02] + ## Apps (Connectors) + Apps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{connect...long as the context suggests usage of available apps. [hash=8c2ddeb1adaf185d] + An app is equivalent to a set of MCP tools within the `codex_apps` MCP. + An installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool...re searchable by `tools_search` will be listed by it. [hash=4f9c0d35da3968a5] + Do not additionally call list_mcp_resources or list_mcp_resource_templates for apps. + + [03] + Deferred tool namespaces: + - mcp__codex_apps__calendar: Plan events and manage your calendar. + - multi_agent_v1: Tools for spawning and managing sub-agents. + +01:message/user: + + + + + + [hash=e8c1de60307d0a3d] + +02:message/user: + Check with bundled guidance. +03:message/developer: + + The previously provided Apps usage instructions no longer apply. + +04:message/user: + Check while guidance is unavailable. + +## Window 3 (after request 2) +Settings: relative to window 2 + model: "apps-a" +-- request 3 (turn) -- +00:message/developer[3]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Escalation Requests + + Commands are run outside the sandbox if they are approved by the user, or match an existing rule that al...hell control operators, including but not limited to: [hash=d24ebf5fa4e4b9bb] + + - Pipes: | + - Logical operators: &&, || + + NEVER provide a prefix_rule if your command uses a heredoc or herestring. + + ### Examples + Good examples of prefixes: + - ["npm", "run", "dev"] + - ["gh", "pr", "check"] + - ["cargo", "test"] + + [02] + ## Apps (Connectors) + Apps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{connect...long as the context suggests usage of available apps. [hash=8c2ddeb1adaf185d] + An app is equivalent to a set of MCP tools within the `codex_apps` MCP. + An installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool...re searchable by `tools_search` will be listed by it. [hash=4f9c0d35da3968a5] + Do not additionally call list_mcp_resources or list_mcp_resource_templates for apps. + + [03] + Deferred tool namespaces: + - mcp__codex_apps__calendar: Plan events and manage your calendar. + - multi_agent_v1: Tools for spawning and managing sub-agents. + +01:message/user: + + + + + + [hash=e8c1de60307d0a3d] + +02:message/user: + Check with bundled guidance. +03:message/developer: + + The previously provided Apps usage instructions no longer apply. + +04:message/user: + Check while guidance is unavailable. +05:message/developer: + Calendar guidance A: {{literal}}. + +06:message/user: + Check the calendar. + +## Window 4 (after request 3) +Settings: relative to window 3 + model: "apps-b" +-- request 4 (turn) -- +00:message/developer[3]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Escalation Requests + + Commands are run outside the sandbox if they are approved by the user, or match an existing rule that al...hell control operators, including but not limited to: [hash=d24ebf5fa4e4b9bb] + + - Pipes: | + - Logical operators: &&, || + + NEVER provide a prefix_rule if your command uses a heredoc or herestring. + + ### Examples + Good examples of prefixes: + - ["npm", "run", "dev"] + - ["gh", "pr", "check"] + - ["cargo", "test"] + + [02] + ## Apps (Connectors) + Apps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{connect...long as the context suggests usage of available apps. [hash=8c2ddeb1adaf185d] + An app is equivalent to a set of MCP tools within the `codex_apps` MCP. + An installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool...re searchable by `tools_search` will be listed by it. [hash=4f9c0d35da3968a5] + Do not additionally call list_mcp_resources or list_mcp_resource_templates for apps. + + [03] + Deferred tool namespaces: + - mcp__codex_apps__calendar: Plan events and manage your calendar. + - multi_agent_v1: Tools for spawning and managing sub-agents. + +01:message/user: + + + + + + [hash=e8c1de60307d0a3d] + +02:message/user: + Check with bundled guidance. +03:message/developer: + + The previously provided Apps usage instructions no longer apply. + +04:message/user: + Check while guidance is unavailable. +05:message/developer: + Calendar guidance A: {{literal}}. + +06:message/user: + Check the calendar. +07:message/developer: + + These Apps usage instructions replace all previously provided Apps usage instructions. + + Calendar guidance B. +08:message/user: + Check again with the new model. +-- request 5 (turn) -- +09:message/user: + + + + + + ...ntry> [hash=8fc9d186c1133657] + +10:message/user: + Check after restarting with the same guidance. + +## Window 5 (after request 5) +Settings: relative to window 4 + model: "apps-clear" +-- request 6 (turn) -- +00:message/developer[3]: + [01] + Filesystem sandboxing defines which files can be read or written. `sandbox_mode` is `read-only`: The san... permits reading files. Network access is restricted. [hash=be7da4425f89053c] + # Escalation Requests + + Commands are run outside the sandbox if they are approved by the user, or match an existing rule that al...hell control operators, including but not limited to: [hash=d24ebf5fa4e4b9bb] + + - Pipes: | + - Logical operators: &&, || + + NEVER provide a prefix_rule if your command uses a heredoc or herestring. + + ### Examples + Good examples of prefixes: + - ["npm", "run", "dev"] + - ["gh", "pr", "check"] + - ["cargo", "test"] + + [02] + ## Apps (Connectors) + Apps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{connect...long as the context suggests usage of available apps. [hash=8c2ddeb1adaf185d] + An app is equivalent to a set of MCP tools within the `codex_apps` MCP. + An installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool...re searchable by `tools_search` will be listed by it. [hash=4f9c0d35da3968a5] + Do not additionally call list_mcp_resources or list_mcp_resource_templates for apps. + + [03] + Deferred tool namespaces: + - mcp__codex_apps__calendar: Plan events and manage your calendar. + - multi_agent_v1: Tools for spawning and managing sub-agents. + +01:message/user: + + + + + + [hash=e8c1de60307d0a3d] + +02:message/user: + Check with bundled guidance. +03:message/developer: + + The previously provided Apps usage instructions no longer apply. + +04:message/user: + Check while guidance is unavailable. +05:message/developer: + Calendar guidance A: {{literal}}. + +06:message/user: + Check the calendar. +07:message/developer: + + These Apps usage instructions replace all previously provided Apps usage instructions. + + Calendar guidance B. +08:message/user: + Check again with the new model. +09:message/user: + + + + + + ...ntry> [hash=8fc9d186c1133657] + +10:message/user: + Check after restarting with the same guidance. +11:message/developer: + + The previously provided Apps usage instructions no longer apply. + +12:message/user: + Clear the guidance. +-- request 7 (turn) -- +13:message/user: + Continue with cleared guidance. +-- request 8 (turn) -- +14:message/user: + + + + + + ...ntry> [hash=1d8e57fdb1985164] + +15:message/user: + Check after restarting with cleared guidance. diff --git a/codex-rs/features/src/lib.rs b/codex-rs/features/src/lib.rs index 6aa0a6e88b..37076ae34a 100644 --- a/codex-rs/features/src/lib.rs +++ b/codex-rs/features/src/lib.rs @@ -278,6 +278,8 @@ pub enum Feature { SpawnCsv, /// Enable apps. Apps, + /// Use Apps usage instructions from the selected model's catalog entry. + CatalogAppsInstructions, /// Route first-party ChatGPT requests through PSP. Psp, /// Enable MCP apps. @@ -1726,6 +1728,12 @@ pub const FEATURES: &[FeatureSpec] = &[ stage: Stage::Stable, default_enabled: true, }, + FeatureSpec { + id: Feature::CatalogAppsInstructions, + key: "catalog_apps_instructions", + stage: Stage::UnderDevelopment, + default_enabled: false, + }, FeatureSpec { id: Feature::Psp, key: "psp", diff --git a/codex-rs/prompts/src/lib.rs b/codex-rs/prompts/src/lib.rs index a83c268817..4202c4aff2 100644 --- a/codex-rs/prompts/src/lib.rs +++ b/codex-rs/prompts/src/lib.rs @@ -15,6 +15,7 @@ pub use guardian_instructions::GuardianClassifierInstructions; pub use guardian_instructions::GuardianPolicyInstructions; pub use guardian_instructions::render_guardian_rejection; pub use model_instructions::render_model_instructions; +pub use model_messages::ResolvedAppsInstructions; pub use model_messages::ResolvedAutoReviewMessages; pub use model_messages::ResolvedCollaborationModeMessages; pub use model_messages::ResolvedGenericPluginMessages; diff --git a/codex-rs/prompts/src/model_messages.rs b/codex-rs/prompts/src/model_messages.rs index ab295566fa..520fdcc793 100644 --- a/codex-rs/prompts/src/model_messages.rs +++ b/codex-rs/prompts/src/model_messages.rs @@ -13,6 +13,7 @@ use codex_protocol::openai_models::ToolMessage; use permissions::ResolvedApprovalMessages; use permissions::ResolvedPermissionMessages; +mod apps; mod collaboration; mod guardian; mod incremental_tools; @@ -20,6 +21,7 @@ mod multi_agent; pub(crate) mod permissions; mod plugins; +pub use apps::ResolvedAppsInstructions; pub use collaboration::ResolvedCollaborationModeMessages; pub use guardian::ResolvedAutoReviewMessages; pub use incremental_tools::ResolvedIncrementalToolMessages; @@ -94,6 +96,15 @@ impl<'a> ResolvedModelMessages<'a> { .and_then(|messages| messages.instructions_template.as_deref()) } + /// Resolves bounded literal Apps wording; callers retain availability and feature gates. + pub fn apps_usage_instructions(&self, mcp_server_name: &str) -> ResolvedAppsInstructions { + ResolvedAppsInstructions::new( + self.catalog_messages + .and_then(|messages| messages.apps.as_ref()), + mcp_server_name, + ) + } + /// Resolves approval messages and their bundled alternatives. pub(crate) fn approvals(self) -> ResolvedApprovalMessages<'a> { ResolvedApprovalMessages::new( @@ -284,6 +295,10 @@ impl<'a> ResolvedModelMessages<'a> { } } +#[cfg(test)] +#[path = "model_messages/apps_tests.rs"] +mod apps_tests; + #[cfg(test)] #[path = "model_messages/plugins_tests.rs"] mod plugins_tests; diff --git a/codex-rs/prompts/src/model_messages/apps.rs b/codex-rs/prompts/src/model_messages/apps.rs new file mode 100644 index 0000000000..ce2319a366 --- /dev/null +++ b/codex-rs/prompts/src/model_messages/apps.rs @@ -0,0 +1,31 @@ +//! Resolves literal Apps wording with a UTF-8 byte limit and the existing bundled namespace. +//! Configured strings are never interpolated or trimmed. + +use codex_protocol::openai_models::AppsMessages; + +// Reserve 200 bytes for markers and transition notices within a 1,000-byte fragment. +const MAX_USAGE_INSTRUCTIONS_BYTES: usize = 800; + +/// Bounded Apps wording and its source, independently of connector availability. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ResolvedAppsInstructions { + pub usage_instructions: String, + pub catalog_override: bool, +} + +impl ResolvedAppsInstructions { + pub(super) fn new(messages: Option<&AppsMessages>, mcp_server_name: &str) -> Self { + let catalog = messages + .and_then(|messages| messages.usage_instructions.as_deref()) + .filter(|text| { + text.is_empty() + || (text.len() <= MAX_USAGE_INSTRUCTIONS_BYTES && !text.trim().is_empty()) + }); + Self { + usage_instructions: catalog.map(str::to_owned).unwrap_or_else(|| format!( + "\n## Apps (Connectors)\nApps (Connectors) can be explicitly triggered in user messages in the format `[$app-name](app://{{connector_id}})`. Apps can also be implicitly triggered as long as the context suggests usage of available apps.\nAn app is equivalent to a set of MCP tools within the `{mcp_server_name}` MCP.\nAn installed app's MCP tools are either provided to you already, or can be lazy-loaded through the `tool_search` tool. If `tool_search` is available, the apps that are searchable by `tools_search` will be listed by it.\nDo not additionally call list_mcp_resources or list_mcp_resource_templates for apps.\n" + )), + catalog_override: catalog.is_some(), + } + } +} diff --git a/codex-rs/prompts/src/model_messages/apps_tests.rs b/codex-rs/prompts/src/model_messages/apps_tests.rs new file mode 100644 index 0000000000..8db9091328 --- /dev/null +++ b/codex-rs/prompts/src/model_messages/apps_tests.rs @@ -0,0 +1,61 @@ +#![cfg(test)] + +use super::ResolvedModelMessages; +use codex_protocol::openai_models::AppsMessages; +use codex_protocol::openai_models::ModelMessages; +use pretty_assertions::assert_eq; + +#[test] +fn apps_resolution_preserves_literal_bytes_and_rejects_invalid_overrides() { + let bundled = ResolvedModelMessages::bundled().apps_usage_instructions("fixture_apps"); + for apps in [ + None, + Some(AppsMessages::default()), + Some(AppsMessages { + usage_instructions: Some(" \t\n".to_string()), + }), + Some(AppsMessages { + usage_instructions: Some("é".repeat(401)), + }), + Some(AppsMessages { + usage_instructions: Some("x".repeat(801)), + }), + ] { + let messages = ModelMessages { + apps, + ..Default::default() + }; + assert_eq!( + ResolvedModelMessages { + catalog_messages: Some(&messages) + } + .apps_usage_instructions("fixture_apps"), + bundled + ); + } +} + +#[test] +fn apps_resolution_keeps_empty_boundary_and_placeholder_text_literal() { + for text in [ + String::new(), + " {{connector_id}}\r\n{{ namespace }} ".to_string(), + "é".repeat(400), + "\u{10ffff}".repeat(200), + ] { + let messages = ModelMessages { + apps: Some(AppsMessages { + usage_instructions: Some(text.clone()), + }), + ..Default::default() + }; + let resolved = ResolvedModelMessages { + catalog_messages: Some(&messages), + } + .apps_usage_instructions("fixture_apps"); + assert_eq!( + (resolved.usage_instructions, resolved.catalog_override), + (text, true) + ); + } +} diff --git a/codex-rs/protocol/src/openai_models.rs b/codex-rs/protocol/src/openai_models.rs index 391acecc55..89b7c3ae3e 100644 --- a/codex-rs/protocol/src/openai_models.rs +++ b/codex-rs/protocol/src/openai_models.rs @@ -596,7 +596,7 @@ impl ModelInfo { /// retained for model catalog compatibility. #[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] pub struct ModelMessages { - /// Optional model-owned Apps wording, transported without runtime interpretation. + /// Model-owned Apps usage guidance, subject to existing Apps availability and authorization. #[serde(default, skip_serializing_if = "Option::is_none")] pub apps: Option, /// Model-owned plugin wording; runtime capability facts and eligibility remain client-owned. @@ -627,11 +627,11 @@ pub struct ModelMessages { pub confirmation_policies: Option, } -/// Optional Apps wording from the model catalog, preserved for future runtime consumption. +/// Literal model-owned Apps guidance inside the existing developer fragment markers. #[derive(Debug, Default, Serialize, Deserialize, Clone, PartialEq, Eq, TS, JsonSchema)] pub struct AppsMessages { - /// Missing and null deserialize as `None`; all strings preserve their exact bytes. - /// Rendering, size limits, and empty-string behavior are defined by consumers. + /// Missing/null uses bundled guidance; empty clears it. Whitespace-only or more than + /// 800 UTF-8 bytes falls back to bundled guidance. Other values preserve exact bytes. #[serde(default, skip_serializing_if = "Option::is_none")] pub usage_instructions: Option, } diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index 8d4b3fe8be..cf4d3d83cf 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -134,6 +134,8 @@ pub const ENVIRONMENTS_INSTRUCTIONS_OPEN_TAG: &str = " boo "", "", "", - "", + APPS_INSTRUCTIONS_OPEN_TAG, + CATALOG_APPS_INSTRUCTIONS_OPEN_TAG, "", "", "", diff --git a/codex-rs/thread-store/src/local/rollout_migration_tests.rs b/codex-rs/thread-store/src/local/rollout_migration_tests.rs index a3730eff10..eb531aa4b0 100644 --- a/codex-rs/thread-store/src/local/rollout_migration_tests.rs +++ b/codex-rs/thread-store/src/local/rollout_migration_tests.rs @@ -942,6 +942,10 @@ async fn migration_rolls_back_response_and_inter_agent_user_boundaries() { "developer", "context only", )), + rollout_response_item(input_response_message( + "developer", + "context only", + )), rollout_response_item(input_response_message( "developer", "context only",