From 3c2bfe7c5d05c33b4d961d692fac9bb888867150 Mon Sep 17 00:00:00 2001 From: sayan-oai Date: Mon, 6 Jul 2026 14:29:19 -0700 Subject: [PATCH] Make Apps guidance react to MCP availability (#30226) ## Why Generic Apps guidance is emitted only while building static initial context. If the Apps MCP is unavailable then and recovers later in the same turn, its tools can become usable without the model receiving the guidance for using them. ## What - move generic Apps guidance into a persisted `apps_instructions` World State section - derive availability from the request's MCP runtime while preserving the existing feature, auth, orchestrator-MCP, and config gates - recognize legacy and retained Apps fragments so resume and compaction do not duplicate guidance - register Apps guidance as rollback-trimmable context - remove the old static injection path and its now-unused connector helper - keep tool construction on its existing independent `list_all_tools()` read rather than adding a request-wide cache; a reconnect between the two reads can differ for one request and reconciles on the next request Apps and plugin guidance now render after the remaining static host-skills block, with Apps before Plugins. ## Testing - `just fmt` - `just test -p codex-core apps_instructions` - `just test -p codex-core apps_guidance_appears_after_background_recovery_within_a_turn` - `just test -p codex-core drop_last_n_user_turns_trims_context_updates_above_rolled_back_turn` --- codex-rs/core/src/apps/render.rs | 5 +- codex-rs/core/src/connectors.rs | 13 -- .../core/src/context/apps_instructions.rs | 10 -- .../context/world_state/apps_instructions.rs | 55 +++++++ .../world_state/apps_instructions_tests.rs | 69 +++++++++ codex-rs/core/src/context/world_state/mod.rs | 2 + .../core/src/context_manager/history_tests.rs | 4 + codex-rs/core/src/event_mapping.rs | 2 + codex-rs/core/src/session/mod.rs | 15 -- codex-rs/core/src/session/world_state.rs | 15 ++ .../core/tests/suite/mcp_tool_exposure.rs | 140 ++++++++++++++++++ codex-rs/core/tests/suite/plugins.rs | 4 +- 12 files changed, 293 insertions(+), 41 deletions(-) create mode 100644 codex-rs/core/src/context/world_state/apps_instructions.rs create mode 100644 codex-rs/core/src/context/world_state/apps_instructions_tests.rs diff --git a/codex-rs/core/src/apps/render.rs b/codex-rs/core/src/apps/render.rs index 2331e13f77..b3d47913f6 100644 --- a/codex-rs/core/src/apps/render.rs +++ b/codex-rs/core/src/apps/render.rs @@ -5,7 +5,10 @@ use codex_protocol::protocol::APPS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::APPS_INSTRUCTIONS_OPEN_TAG; pub(crate) fn render_apps_section(connectors: &[AppInfo]) -> Option { - AppsInstructions::from_connectors(connectors).map(|instructions| instructions.render()) + connectors + .iter() + .any(|connector| connector.is_accessible && connector.is_enabled) + .then(|| AppsInstructions.render()) } #[cfg(test)] diff --git a/codex-rs/core/src/connectors.rs b/codex-rs/core/src/connectors.rs index 805d56b648..6e643f34d5 100644 --- a/codex-rs/core/src/connectors.rs +++ b/codex-rs/core/src/connectors.rs @@ -80,19 +80,6 @@ pub async fn list_accessible_connectors_from_mcp_tools( ) } -pub(crate) async fn list_accessible_and_enabled_connectors_from_manager( - mcp_connection_manager: &McpConnectionManager, - config: &Config, -) -> Vec { - with_app_enabled_state( - accessible_connectors_from_mcp_tools(&mcp_connection_manager.list_all_tools().await), - config, - ) - .into_iter() - .filter(|connector| connector.is_accessible && connector.is_enabled) - .collect() -} - #[instrument(level = "trace", skip_all)] pub(crate) async fn list_tool_suggest_discoverable_tools_with_auth( config: &Config, diff --git a/codex-rs/core/src/context/apps_instructions.rs b/codex-rs/core/src/context/apps_instructions.rs index 761187c794..71d836d4ea 100644 --- a/codex-rs/core/src/context/apps_instructions.rs +++ b/codex-rs/core/src/context/apps_instructions.rs @@ -1,4 +1,3 @@ -use crate::connectors::AppInfo; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; use codex_protocol::protocol::APPS_INSTRUCTIONS_CLOSE_TAG; use codex_protocol::protocol::APPS_INSTRUCTIONS_OPEN_TAG; @@ -8,15 +7,6 @@ use super::ContextualUserFragment; #[derive(Debug, Clone, PartialEq)] pub(crate) struct AppsInstructions; -impl AppsInstructions { - pub(crate) fn from_connectors(connectors: &[AppInfo]) -> Option { - connectors - .iter() - .any(|connector| connector.is_accessible && connector.is_enabled) - .then_some(Self) - } -} - impl ContextualUserFragment for AppsInstructions { fn role(&self) -> &'static str { "developer" diff --git a/codex-rs/core/src/context/world_state/apps_instructions.rs b/codex-rs/core/src/context/world_state/apps_instructions.rs new file mode 100644 index 0000000000..767e1c584e --- /dev/null +++ b/codex-rs/core/src/context/world_state/apps_instructions.rs @@ -0,0 +1,55 @@ +use super::PreviousSectionState; +use super::WorldStateSection; +use crate::context::AppsInstructions; +use crate::context::ContextualUserFragment; + +/// Whether generic Apps usage guidance should be visible to the model. +#[derive(Clone, Copy, Debug, Default)] +pub(crate) struct AppsInstructionsState { + available: bool, +} + +impl AppsInstructionsState { + pub(crate) fn new(available: bool) -> Self { + Self { available } + } +} + +impl WorldStateSection for AppsInstructionsState { + const ID: &'static str = "apps_instructions"; + type Snapshot = bool; + + fn snapshot(&self) -> Self::Snapshot { + self.available + } + + fn matches_legacy_fragment(role: &str, text: &str) -> bool { + role == "developer" && AppsInstructions::matches_text(text) + } + + fn has_retained_fragment_matcher() -> bool { + true + } + + fn matches_retained_fragment(role: &str, text: &str) -> bool { + Self::matches_legacy_fragment(role, text) + } + + fn render_diff( + &self, + previous: PreviousSectionState<'_, Self::Snapshot>, + ) -> Option> { + if !self.available + || matches!(previous, PreviousSectionState::Known(previous) if *previous) + || matches!(previous, PreviousSectionState::Unknown) + { + return None; + } + + Some(Box::new(AppsInstructions)) + } +} + +#[cfg(test)] +#[path = "apps_instructions_tests.rs"] +mod tests; 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 new file mode 100644 index 0000000000..92f6613594 --- /dev/null +++ b/codex-rs/core/src/context/world_state/apps_instructions_tests.rs @@ -0,0 +1,69 @@ +use super::*; +use crate::context::ContextualUserFragment; +use codex_protocol::models::ResponseItem; +use pretty_assertions::assert_eq; + +fn render(state: AppsInstructionsState, previous: PreviousSectionState<'_, bool>) -> Vec { + state + .render_diff(previous) + .into_iter() + .map(|fragment| fragment.render()) + .collect() +} + +#[test] +fn renders_only_when_apps_become_available() { + let unavailable = AppsInstructionsState::new(/*available*/ false); + let available = AppsInstructionsState::new(/*available*/ true); + let false_snapshot = false; + let true_snapshot = true; + + assert_eq!( + render(unavailable, PreviousSectionState::Absent), + Vec::::new() + ); + assert_eq!(render(available, PreviousSectionState::Absent).len(), 1); + assert_eq!( + render(available, PreviousSectionState::Known(&false_snapshot)).len(), + 1 + ); + assert_eq!( + render(available, PreviousSectionState::Known(&true_snapshot)), + Vec::::new() + ); + assert_eq!( + render(unavailable, PreviousSectionState::Known(&true_snapshot)), + Vec::::new() + ); +} + +#[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); + + assert!( + world_state + .render_history_diff(/*previous*/ None, &[legacy]) + .is_empty() + ); +} + +#[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.snapshot(); + let retained: ResponseItem = ContextualUserFragment::into(AppsInstructions); + + assert_eq!( + world_state.render_history_diff(Some(&snapshot), &[]).len(), + 1 + ); + assert!( + world_state + .render_history_diff(Some(&snapshot), &[retained]) + .is_empty() + ); +} diff --git a/codex-rs/core/src/context/world_state/mod.rs b/codex-rs/core/src/context/world_state/mod.rs index 5dc22030ba..ccdc976c4c 100644 --- a/codex-rs/core/src/context/world_state/mod.rs +++ b/codex-rs/core/src/context/world_state/mod.rs @@ -1,4 +1,5 @@ mod agents_md; +mod apps_instructions; mod environment; mod plugins_instructions; @@ -17,6 +18,7 @@ use std::collections::BTreeMap; use std::fmt; pub(crate) use agents_md::AgentsMdState; +pub(crate) use apps_instructions::AppsInstructionsState; pub(crate) use environment::EnvironmentsState; pub(crate) use plugins_instructions::PluginsInstructionsState; diff --git a/codex-rs/core/src/context_manager/history_tests.rs b/codex-rs/core/src/context_manager/history_tests.rs index df5abd4708..679e1a69f7 100644 --- a/codex-rs/core/src/context_manager/history_tests.rs +++ b/codex-rs/core/src/context_manager/history_tests.rs @@ -20,6 +20,7 @@ use codex_protocol::models::ReasoningItemContent; use codex_protocol::models::ReasoningItemReasoningSummary; use codex_protocol::openai_models::InputModality; use codex_protocol::openai_models::default_input_modalities; +use codex_protocol::protocol::APPS_INSTRUCTIONS_OPEN_TAG; use codex_protocol::protocol::AskForApproval; use codex_protocol::protocol::InterAgentCommunication; use codex_protocol::protocol::PLUGINS_INSTRUCTIONS_OPEN_TAG; @@ -1022,6 +1023,9 @@ fn drop_last_n_user_turns_trims_context_updates_above_rolled_back_turn() { user_input_text_msg("turn 1 user"), assistant_msg("turn 1 assistant"), developer_msg("Generated images are saved to /tmp as /tmp/image-1.png by default."), + developer_msg(&format!( + "{APPS_INSTRUCTIONS_OPEN_TAG}\nROLLED_BACK_APPS_INSTRUCTIONS" + )), 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 5e24b78d05..ef8f0fad5b 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::is_image_close_tag_text; use codex_protocol::models::is_image_open_tag_text; use codex_protocol::models::is_local_image_close_tag_text; use codex_protocol::models::is_local_image_open_tag_text; +use codex_protocol::protocol::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; @@ -32,6 +33,7 @@ use crate::web_search::web_search_action_detail; const CONTEXTUAL_DEVELOPER_PREFIXES: &[&str] = &[ "", "", + APPS_INSTRUCTIONS_OPEN_TAG, COLLABORATION_MODE_OPEN_TAG, MULTI_AGENT_MODE_OPEN_TAG, PLUGINS_INSTRUCTIONS_OPEN_TAG, diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index d4cfd9e05d..ba8eacb395 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -21,9 +21,7 @@ use crate::build_available_skills; use crate::compact; use crate::config::ManagedFeatures; use crate::config::resolve_tool_suggest_config_from_layer_stack; -use crate::connectors; use crate::context::ApprovedCommandPrefixSaved; -use crate::context::AppsInstructions; use crate::context::AvailableSkillsInstructions; use crate::context::CollaborationModeInstructions; use crate::context::ContextualUserFragment; @@ -3235,19 +3233,6 @@ impl Session { .push(PersonalitySpecInstructions::new(personality_message).render()); } } - if turn_context.config.include_apps_instructions && turn_context.apps_enabled() { - let accessible_and_enabled_connectors = - connectors::list_accessible_and_enabled_connectors_from_manager( - mcp.manager(), - &turn_context.config, - ) - .await; - if let Some(apps_instructions) = - AppsInstructions::from_connectors(&accessible_and_enabled_connectors) - { - developer_sections.push(apps_instructions.render()); - } - } if turn_context.config.include_skill_instructions { let available_skills = build_available_skills( turn_context.turn_skills.snapshot.outcome(), diff --git a/codex-rs/core/src/session/world_state.rs b/codex-rs/core/src/session/world_state.rs index 89684e9e50..a0d93cac83 100644 --- a/codex-rs/core/src/session/world_state.rs +++ b/codex-rs/core/src/session/world_state.rs @@ -1,6 +1,8 @@ use super::session::Session; use super::step_context::StepContext; +use crate::connectors; use crate::context::world_state::AgentsMdState; +use crate::context::world_state::AppsInstructionsState; use crate::context::world_state::EnvironmentsState; use crate::context::world_state::PluginsInstructionsState; use crate::context::world_state::WorldState; @@ -37,6 +39,19 @@ impl Session { .with_subagents(environment_subagents), ); } + let apps_available = + if turn_context.config.include_apps_instructions && turn_context.apps_enabled() { + let tools = step_context.mcp.manager().list_all_tools().await; + connectors::with_app_enabled_state( + connectors::accessible_connectors_from_mcp_tools(&tools), + &turn_context.config, + ) + .into_iter() + .any(|connector| connector.is_accessible && connector.is_enabled) + } else { + false + }; + world_state.add_section(AppsInstructionsState::new(apps_available)); world_state.add_section(PluginsInstructionsState::new( step_context.mcp.plugins_available(), )); diff --git a/codex-rs/core/tests/suite/mcp_tool_exposure.rs b/codex-rs/core/tests/suite/mcp_tool_exposure.rs index 32ecf7cf2a..51ab7842ee 100644 --- a/codex-rs/core/tests/suite/mcp_tool_exposure.rs +++ b/codex-rs/core/tests/suite/mcp_tool_exposure.rs @@ -1,22 +1,32 @@ use anyhow::Result; use codex_features::Feature; use codex_mcp::CODEX_APPS_MCP_SERVER_NAME; +use codex_protocol::protocol::EventMsg; use codex_protocol::protocol::McpServerRefreshConfig; use codex_protocol::protocol::Op; +use codex_protocol::request_user_input::RequestUserInputAnswer; +use codex_protocol::request_user_input::RequestUserInputResponse; +use codex_protocol::user_input::UserInput; use core_test_support::apps_test_server::AppsTestServer; use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL; use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE; +use core_test_support::apps_test_server::apps_enabled_builder; use core_test_support::apps_test_server::search_capable_apps_builder; use core_test_support::responses; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; +use core_test_support::responses::ev_function_call; use core_test_support::responses::ev_response_created; use core_test_support::responses::mount_sse_sequence; use core_test_support::responses::namespace_child_tool; use core_test_support::responses::sse; use core_test_support::skip_if_no_network; +use core_test_support::wait_for_event; +use core_test_support::wait_for_event_match; use core_test_support::wait_for_mcp_server; use serde_json::Value; +use serde_json::json; +use std::collections::HashMap; use std::time::Duration; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] @@ -86,6 +96,136 @@ async fn code_mode_only_exposes_direct_model_only_mcp_namespaces() -> Result<()> Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apps_guidance_appears_after_background_recovery_within_a_turn() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = responses::start_mock_server().await; + let (apps_server, startup_control) = + AppsTestServer::mount_with_startup_control(&server).await?; + // Initial context is rendered twice before the first request, so keep both reads unavailable. + startup_control.fail_next_initialize_attempts(/*attempts*/ 2); + let call_id = "pause-for-apps"; + let response = mount_sse_sequence( + &server, + vec![ + sse(vec![ + ev_response_created("resp-1"), + ev_function_call( + call_id, + "request_user_input", + &json!({ + "questions": [{ + "id": "continue", + "header": "Continue", + "question": "Continue after Apps recovers?", + "options": [{ + "label": "Yes (Recommended)", + "description": "Continue the test." + }, { + "label": "No", + "description": "Stop the test." + }] + }] + }) + .to_string(), + ), + ev_completed("resp-1"), + ]), + sse(vec![ + ev_response_created("resp-2"), + ev_assistant_message("msg-2", "done"), + ev_completed("resp-2"), + ]), + ], + ) + .await; + let mut builder = + apps_enabled_builder(apps_server.chatgpt_base_url.clone()).with_config(|config| { + config + .features + .enable(Feature::DeferredExecutor) + .expect("test config should allow feature update"); + config + .features + .enable(Feature::DefaultModeRequestUserInput) + .expect("test config should allow feature update"); + }); + let test = builder.build(&server).await?; + let mcp_runtime = test.codex.current_mcp_runtime().await; + + test.codex + .submit(Op::UserInput { + items: vec![UserInput::Text { + text: "use an app after it recovers".into(), + text_elements: Vec::new(), + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + let request = wait_for_event_match(&test.codex, |event| match event { + EventMsg::RequestUserInput(request) => Some(request.clone()), + _ => None, + }) + .await; + + let initial_requests = response.requests(); + assert_eq!(initial_requests.len(), 1); + let initial_request = &initial_requests[0]; + assert_eq!( + initial_request + .message_input_texts("developer") + .iter() + .filter(|text| text.contains("")) + .count(), + 0 + ); + + tokio::time::timeout(Duration::from_secs(3), async { + loop { + if !mcp_runtime.manager().list_all_tools().await.is_empty() { + break; + } + tokio::time::sleep(Duration::from_millis(10)).await; + } + }) + .await + .expect("Apps MCP should recover while the turn is paused"); + test.codex + .submit(Op::UserInputAnswer { + id: request.turn_id, + response: RequestUserInputResponse { + answers: HashMap::from([( + "continue".to_string(), + RequestUserInputAnswer { + answers: vec!["Yes (Recommended)".to_string()], + }, + )]), + }, + }) + .await?; + wait_for_event(&test.codex, |event| { + matches!(event, EventMsg::TurnComplete(_)) + }) + .await; + + let requests = response.requests(); + assert_eq!(requests.len(), 2); + assert_eq!( + requests[1] + .message_input_texts("developer") + .iter() + .filter(|text| text.contains("")) + .count(), + 1 + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn later_follow_up_uses_background_recovered_apps_after_mid_thread_startup_failures() -> Result<()> { diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index b3420feabe..aaf65baad0 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -255,8 +255,8 @@ async fn capability_sections_render_in_developer_message_in_order() -> Result<() .find("## Plugins") .expect("expected plugins section in developer message"); assert!( - apps_pos < skills_pos && skills_pos < plugins_pos, - "expected Apps -> Skills -> Plugins order: {developer_messages:?}" + skills_pos < apps_pos && apps_pos < plugins_pos, + "expected Skills -> Apps -> Plugins order: {developer_messages:?}" ); assert!( !developer_text.contains("`sample`: inspect sample data"),