mirror of
https://github.com/openai/codex.git
synced 2026-09-05 15:18:41 +00:00
fix(core): avoid app wait for unrelated skills
This commit is contained in:
@@ -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::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
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::<HashSet<_>>();
|
||||
let remaining_mentioned_skills = mentioned_skills
|
||||
.iter()
|
||||
.filter(|skill| !structured_skill_paths.contains(&skill.path_to_skills_md))
|
||||
.cloned()
|
||||
.collect::<Vec<_>>();
|
||||
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,
|
||||
|
||||
@@ -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("<name>sample:sample-search</name>")),
|
||||
"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(()));
|
||||
|
||||
Reference in New Issue
Block a user