From ae49b11234a2fa0ea67db4c7a35a3d94e4a169da Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Thu, 28 May 2026 15:55:30 -0300 Subject: [PATCH] fix(core): avoid app wait for unrelated skills --- codex-rs/core/src/session/turn.rs | 74 ++++++++++++++++++++++++---- codex-rs/core/tests/suite/plugins.rs | 56 +++++++++++++++++++++ 2 files changed, 121 insertions(+), 9 deletions(-) diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index f37a2f6367..d07735ffcd 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -26,6 +26,7 @@ use crate::hook_runtime::run_pending_session_start_hooks; use crate::hook_runtime::run_turn_stop_hooks; use crate::injection::ToolMentionKind; use crate::injection::app_id_from_path; +use crate::injection::extract_tool_mentions; use crate::injection::tool_kind_for_path; use crate::mcp_skill_dependencies::maybe_prompt_and_install_mcp_dependencies; use crate::mcp_tool_exposure::build_mcp_tool_exposure; @@ -484,12 +485,43 @@ async fn build_skills_and_plugins( collect_explicit_plugin_mentions(&user_input, loaded_plugins.capability_summaries()); let explicitly_mentioned_apps = collect_explicit_app_ids(&user_input); let skills_outcome = turn_context.turn_skills.outcome.as_ref(); - // Skill injection can contain app references, and plain skill mentions share - // resolution space with apps. Preserve explicit-turn behavior by resolving - // app inventory before finalizing any selected skill. - let has_explicit_skill_selection = turn_context.apps_enabled() + // Structured skill selections are unambiguous without app inventory. Load + // them before deciding whether the pending Apps MCP is a dependency of this + // turn, then reuse the injections below so loading and telemetry happen once. + let structured_skill_input = user_input + .iter() + .filter(|item| matches!(item, UserInput::Skill { .. })) + .cloned() + .collect::>(); + let structured_mentioned_skills = collect_explicit_skill_mentions( + &structured_skill_input, + &skills_outcome.skills, + &skills_outcome.disabled_paths, + &HashMap::new(), + ); + let SkillInjections { + items: mut skill_injections, + warnings: mut skill_warnings, + } = build_skill_injections( + &structured_mentioned_skills, + Some(skills_outcome), + Some(&turn_context.session_telemetry), + &sess.services.analytics_events_client, + tracking.clone(), + ) + .await; + let structured_skill_may_reference_apps = + skill_injections_may_reference_apps(&skill_injections); + // Plain text skill mentions can collide with app slugs, so preserve their + // existing app-inventory resolution behavior. + let text_skill_input = user_input + .iter() + .filter(|item| matches!(item, UserInput::Text { .. })) + .cloned() + .collect::>(); + let text_may_select_skill_requiring_app_resolution = turn_context.apps_enabled() && !collect_explicit_skill_mentions( - &user_input, + &text_skill_input, &skills_outcome.skills, &skills_outcome.disabled_paths, &HashMap::new(), @@ -505,7 +537,8 @@ async fn build_skills_and_plugins( if turn_context.apps_enabled() && (!explicitly_mentioned_apps.is_empty() || mentioned_plugin_uses_apps - || has_explicit_skill_selection) + || structured_skill_may_reference_apps + || text_may_select_skill_requiring_app_resolution) { explicitly_requested_mcp_servers.insert(CODEX_APPS_MCP_SERVER_NAME.to_string()); } @@ -583,17 +616,28 @@ async fn build_skills_and_plugins( .await?; } + let structured_skill_paths = structured_mentioned_skills + .iter() + .map(|skill| &skill.path_to_skills_md) + .collect::>(); + let remaining_mentioned_skills = mentioned_skills + .iter() + .filter(|skill| !structured_skill_paths.contains(&skill.path_to_skills_md)) + .cloned() + .collect::>(); let SkillInjections { - items: skill_injections, - warnings: skill_warnings, + items: remaining_skill_injections, + warnings: remaining_skill_warnings, } = build_skill_injections( - &mentioned_skills, + &remaining_mentioned_skills, Some(skills_outcome), Some(&turn_context.session_telemetry), &sess.services.analytics_events_client, tracking.clone(), ) .await; + skill_injections.extend(remaining_skill_injections); + skill_warnings.extend(remaining_skill_warnings); for message in skill_warnings { sess.send_event(turn_context, EventMsg::Warning(WarningEvent { message })) @@ -727,6 +771,18 @@ async fn wait_for_explicit_mcp_servers( Some(()) } +fn skill_injections_may_reference_apps( + skill_injections: &[crate::injection::SkillInjection], +) -> bool { + skill_injections.iter().any(|skill| { + let mentions = extract_tool_mentions(&skill.contents); + mentions.plain_names().next().is_some() + || mentions + .paths() + .any(|path| tool_kind_for_path(path) == ToolMentionKind::App) + }) +} + async fn track_turn_resolved_config_analytics( sess: &Session, turn_context: &TurnContext, diff --git a/codex-rs/core/tests/suite/plugins.rs b/codex-rs/core/tests/suite/plugins.rs index c743bb9034..c7e330d52f 100644 --- a/codex-rs/core/tests/suite/plugins.rs +++ b/codex-rs/core/tests/suite/plugins.rs @@ -439,6 +439,62 @@ async fn explicitly_selected_skill_waits_for_pending_apps_startup() -> Result<() Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn explicitly_selected_non_app_skill_does_not_wait_for_pending_apps_startup() -> Result<()> { + skip_if_no_network!(Ok(())); + let server = start_mock_server().await; + let apps_server = AppsTestServer::mount_with_connector_name_and_tools_list_delay( + &server, + "Google Calendar", + Some(Duration::from_secs(/*secs*/ 5)), + ) + .await?; + let mock = mount_sse_once( + &server, + sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]), + ) + .await; + + let codex_home = Arc::new(TempDir::new()?); + let skill_path = write_plugin_skill_plugin(codex_home.as_ref()); + let codex = + build_apps_enabled_plugin_test_codex(&server, codex_home, apps_server.chatgpt_base_url) + .await?; + + let completed = tokio::time::timeout(Duration::from_secs(/*secs*/ 2), async { + codex + .submit(Op::UserInput { + environments: None, + items: vec![codex_protocol::user_input::UserInput::Skill { + name: "sample:sample-search".into(), + path: skill_path, + }], + final_output_json_schema: None, + responsesapi_client_metadata: None, + additional_context: Default::default(), + thread_settings: Default::default(), + }) + .await?; + wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await; + Ok::<_, anyhow::Error>(()) + }) + .await; + completed + .expect("non-app skill selection should not wait for apps startup") + .expect("non-app skill turn should complete"); + + let request = mock.single_request(); + assert!( + request + .message_input_texts("user") + .iter() + .any(|text| text.contains("sample:sample-search")), + "expected selected non-app skill to be injected on the first turn" + ); + + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn selected_skill_rewaits_for_app_after_installing_mcp_dependency() -> Result<()> { skip_if_no_network!(Ok(()));