From a309f9206aacc2feffde83ab7d8c50527fddcae3 Mon Sep 17 00:00:00 2001 From: Anthony Tafoya Date: Mon, 19 Oct 2026 16:53:32 +0000 Subject: [PATCH] Integrate discovery V2 with step-scoped skill catalogs (#55322) ## What changed - Build executor skill catalogs from discovery V2 inventories without reopening host paths or falling back to legacy discovery. - Merge Cloud and executor skills by remote plugin identity and skill name, retaining Cloud metadata and accepted provider read routes. Apply plugin selection and disablement before merging. - Share a stable catalog across each step's rendering, list/read tools, and explicit and implicit skill invocations. - Preserve unmatched executor entries when Cloud catalogs are unavailable or partial; only complete Cloud catalogs disable unmatched remote entries. - Bind discovery caches to captured environments and executor sessions, and refresh MCP when V2 inventories change. Preserve pending invalidations when forced MCP refreshes fail. ## Testing Add regression coverage for V2 inventory sharing and refresh, environment isolation, catalog merging, partial Cloud results, disabled plugins, and catalog consistency across steps and read routes. GitOrigin-RevId: 842bdcbc9e19364dd69a9abf9e48330b7425a76c --- codex-rs/Cargo.lock | 1 + codex-rs/core-plugins/src/catalog.rs | 12 +- codex-rs/core-plugins/src/catalog_tests.rs | 35 +- codex-rs/core-plugins/src/executor_catalog.rs | 1 + codex-rs/core/src/mcp.rs | 2 +- codex-rs/core/src/session/mcp.rs | 114 ++- codex-rs/core/src/session/mcp_refresh.rs | 4 + .../core/src/session/mcp_refresh_tests.rs | 27 + codex-rs/core/src/session/mod.rs | 19 +- codex-rs/core/src/session/tests.rs | 8 +- codex-rs/core/src/session/turn.rs | 4 +- codex-rs/core/src/skills.rs | 4 +- codex-rs/core/src/thread_manager_tests.rs | 36 + .../handlers/unified_exec/exec_command.rs | 2 +- .../core/tests/suite/executor_discovery_v2.rs | 260 +++++- .../suite/remote_env_required_skills_tests.rs | 1 + .../suite/scenarios_skill_catalog_dedup.rs | 1 + codex-rs/core/tests/suite/skills_extension.rs | 10 + .../skills_extension/cloud_lifecycle_tests.rs | 1 + .../skills_extension/cloud_skill_tests.rs | 4 + .../skills_extension/yielded_skill_tests.rs | 1 + .../exec-server-protocol/src/capabilities.rs | 12 + .../src/capabilities_tests.rs | 2 +- .../src/capability_discovery_cache.rs | 327 +++++--- .../src/discoverV2/capability_discoveries.rs | 15 +- codex-rs/exec-server/tests/environment.rs | 105 ++- .../ext/extension-api/src/contributors.rs | 9 +- .../src/contributors/turn_input.rs | 2 + codex-rs/ext/mcp/src/cloud_plugin_tests.rs | 4 +- .../ext/mcp/src/plugin_contributor_state.rs | 14 +- .../mcp/src/plugin_contributor_state_tests.rs | 54 +- codex-rs/ext/mcp/tests/plugin_mcp.rs | 13 +- codex-rs/ext/skills/Cargo.toml | 1 + codex-rs/ext/skills/src/catalog.rs | 164 +++- codex-rs/ext/skills/src/catalog_tests.rs | 77 ++ codex-rs/ext/skills/src/cloud_cache_tests.rs | 125 ++- codex-rs/ext/skills/src/cloud_skill.rs | 11 +- .../character_routing_card_tests.rs | 1 + codex-rs/ext/skills/src/extension.rs | 166 ++-- codex-rs/ext/skills/src/invocation.rs | 83 +- codex-rs/ext/skills/src/invocation_tests.rs | 18 +- codex-rs/ext/skills/src/lib.rs | 1 + codex-rs/ext/skills/src/provider.rs | 5 +- codex-rs/ext/skills/src/provider/executor.rs | 5 + .../src/provider/executor/discovery_v2.rs | 97 +++ codex-rs/ext/skills/src/provider/host.rs | 1 + codex-rs/ext/skills/src/render_tests.rs | 24 +- codex-rs/ext/skills/src/required.rs | 62 +- codex-rs/ext/skills/src/selection.rs | 15 +- .../experiment_tests.rs | 2 + .../src/shadow_selection_experiment/mod.rs | 2 +- .../ext/skills/src/skills_extension_state.rs | 173 +++- codex-rs/ext/skills/src/sources.rs | 4 +- codex-rs/ext/skills/src/state.rs | 163 ++-- codex-rs/ext/skills/src/tools/list.rs | 2 +- codex-rs/ext/skills/src/tools/mod.rs | 106 +-- codex-rs/ext/skills/src/tools/read.rs | 69 +- .../ext/skills/src/world_state_catalogs.rs | 114 +-- .../tests/executor_file_system_authority.rs | 144 +++- codex-rs/ext/skills/tests/skills_extension.rs | 559 ++++++++++++- .../skills_extension/discovery_v2_tests.rs | 280 +++++++ .../skills_extension/merge_catalog_tests.rs | 780 ++++++++++++++++++ .../presentation_dedup_tests.rs | 1 + .../shadow_task_context_tests.rs | 2 + 64 files changed, 3627 insertions(+), 734 deletions(-) create mode 100644 codex-rs/core/src/session/mcp_refresh_tests.rs create mode 100644 codex-rs/ext/skills/src/catalog_tests.rs create mode 100644 codex-rs/ext/skills/src/provider/executor/discovery_v2.rs create mode 100644 codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs create mode 100644 codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 520004d7c3..51d0d6ec92 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -4693,6 +4693,7 @@ dependencies = [ "codex-analytics", "codex-attachment-store", "codex-config", + "codex-core-plugin-common", "codex-exec-server", "codex-extension-api", "codex-mcp", diff --git a/codex-rs/core-plugins/src/catalog.rs b/codex-rs/core-plugins/src/catalog.rs index a5c844e28f..06f847c390 100644 --- a/codex-rs/core-plugins/src/catalog.rs +++ b/codex-rs/core-plugins/src/catalog.rs @@ -88,8 +88,8 @@ pub enum PluginSourceLocation { } /// Uses Cloud membership and metadata, retaining version-matched installations in order. -/// Unavailable Cloud data preserves executor enablement. With a Cloud catalog, unmatched remote -/// plugins are retained but disabled. Local plugins retain their existing enablement. +/// Unavailable or partial Cloud data preserves unmatched executor entries. Complete Cloud +/// catalogs disable unmatched remote entries; local entries retain their enablement. pub fn merge_catalogs( executor: Option<&PluginCatalog>, cloud: Option<&PluginCatalog>, @@ -113,9 +113,11 @@ pub fn merge_catalogs( .iter_mut() .find(|definition| definition.id == installation.id) else { - let mut disabled = installation.clone(); - disabled.enabled = false; - merged.entries.push(disabled); + let mut unmatched = installation.clone(); + if !cloud.is_partial { + unmatched.enabled = false; + } + merged.entries.push(unmatched); continue; }; if definition.version != installation.version { diff --git a/codex-rs/core-plugins/src/catalog_tests.rs b/codex-rs/core-plugins/src/catalog_tests.rs index 7a68869c9f..81ec37636e 100644 --- a/codex-rs/core-plugins/src/catalog_tests.rs +++ b/codex-rs/core-plugins/src/catalog_tests.rs @@ -125,7 +125,11 @@ fn cloud_catalog_controls_remote_plugins_but_preserves_local_plugins() -> anyhow assert_eq!( merge_catalogs(Some(&unmatched), cloud.as_ref()), PluginCatalog { - entries: disabled_entries.clone(), + entries: if cloud.as_ref().is_some_and(|catalog| catalog.is_partial) { + unmatched.entries.clone() + } else { + disabled_entries.clone() + }, is_partial: cloud.as_ref().is_some_and(|catalog| catalog.is_partial), warnings: cloud.map(|catalog| catalog.warnings).unwrap_or_default(), }, @@ -162,3 +166,32 @@ fn cloud_catalog_controls_remote_plugins_but_preserves_local_plugins() -> anyhow ); Ok(()) } + +#[test] +fn partial_cloud_merges_known_plugins_and_preserves_unmatched_enablement() -> anyhow::Result<()> { + let definition = cloud_plugin("matched"); + let matched = executor_plugin("matched", "executor-a")?; + let mut denied = executor_plugin("denied", "executor-a")?; + denied.enabled = false; + let unmatched = executor_plugin("unmatched", "executor-a")?; + let executor = PluginCatalog { + entries: vec![matched.clone(), denied.clone(), unmatched.clone()], + ..Default::default() + }; + let cloud = PluginCatalog { + entries: vec![definition.clone()], + is_partial: true, + ..Default::default() + }; + let mut expected = definition; + expected.locations.extend(matched.locations); + assert_eq!( + merge_catalogs(Some(&executor), Some(&cloud)), + PluginCatalog { + entries: vec![expected, denied, unmatched], + is_partial: true, + ..Default::default() + } + ); + Ok(()) +} diff --git a/codex-rs/core-plugins/src/executor_catalog.rs b/codex-rs/core-plugins/src/executor_catalog.rs index 355e2bc596..33425f0e8a 100644 --- a/codex-rs/core-plugins/src/executor_catalog.rs +++ b/codex-rs/core-plugins/src/executor_catalog.rs @@ -18,6 +18,7 @@ pub fn catalog_from_snapshots( let inventory = match &snapshot.result { Ok(inventory) => inventory, Err(error) => { + catalog.is_partial = true; catalog.warnings.push(format!( "could not discover plugins in environment `{}`; skipping its plugin inventory: {error}", snapshot.environment_id diff --git a/codex-rs/core/src/mcp.rs b/codex-rs/core/src/mcp.rs index ccc96cc488..eaf301eb4e 100644 --- a/codex-rs/core/src/mcp.rs +++ b/codex-rs/core/src/mcp.rs @@ -232,7 +232,6 @@ impl McpManager { || disabled_plugin_ids.iter().any(|id| id == identity.as_str()) { selected.disabled_plugin_roots.push(selected_root_id); - selected.disabled_plugin_ids.push(plugin_id); } else { selected.plugins.push(SelectedPluginIdentity { selected_root_id: Some(selected_root_id), @@ -519,6 +518,7 @@ impl McpManager { fn available_plugin_roots(context: McpServerContributionContext<'_, Config>) -> HashSet { let mut roots = context .ready_selected_capability_roots() + .filter(|_| context.executor_capability_discovery_v2().is_none()) .unwrap_or_default() .iter() .map(|root| root.id.clone()) diff --git a/codex-rs/core/src/session/mcp.rs b/codex-rs/core/src/session/mcp.rs index 4b00bfd292..38a7fe82ae 100644 --- a/codex-rs/core/src/session/mcp.rs +++ b/codex-rs/core/src/session/mcp.rs @@ -125,16 +125,19 @@ impl Session { .await; let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); - let executor_capability_discovery_v2 = self + let (executor_capability_discovery_v2, inventory_changed) = self .executor_capability_discovery_for_step_v2(config, &environments) .await; - let executor_capability_discovery = self + let (executor_capability_discovery, discovery_recovered) = self .executor_capability_discovery_for_step( config, - &ready_selected_capability_roots, + &selected_capability_roots, &environments, ) .await; + if inventory_changed || discovery_recovered { + self.mark_mcp_runtime_dirty(); + } let mcp_projection = self.services .mcp_manager @@ -236,13 +239,13 @@ impl Session { .await; let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); - let executor_capability_discovery_v2 = self + let (executor_capability_discovery_v2, _) = self .executor_capability_discovery_for_step_v2(&desired.config, &desired.environments) .await; - let executor_capability_discovery = self + let (executor_capability_discovery, _) = self .executor_capability_discovery_for_step( &desired.config, - &ready_selected_capability_roots, + &selected_capability_roots, &desired.environments, ) .await; @@ -298,12 +301,17 @@ impl Session { pub(crate) async fn hard_refresh_latest_codex_apps_tools( self: &Arc, ) -> anyhow::Result> { - self.refresh_mcp_if_dirty().await; let _refresh = self .mcp_refresh .acquire() .await .map_err(|_| anyhow::anyhow!("MCP runtime refresh semaphore closed"))?; + // This is forced work even when there was no pending invalidation. + self.mcp_refresh.claim(); + let mut refresh_invalidation = McpRefreshInvalidationGuard { + refresh: &self.mcp_refresh, + published: false, + }; let auth = self.services.auth_manager.auth().await; let environments = self.services.turn_environments.snapshot().await; let environment_selections = environments.all_selections(); @@ -313,13 +321,13 @@ impl Session { .await; let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); - let executor_capability_discovery_v2 = self + let (executor_capability_discovery_v2, _) = self .executor_capability_discovery_for_step_v2(&desired.config, &desired.environments) .await; - let executor_capability_discovery = self + let (executor_capability_discovery, _) = self .executor_capability_discovery_for_step( &desired.config, - &ready_selected_capability_roots, + &selected_capability_roots, &desired.environments, ) .await; @@ -367,6 +375,7 @@ impl Session { .scope(self.services.mcp_runtime.replace_fresh(input)) .await; self.services.thread_extension_data.insert(selected_plugins); + refresh_invalidation.published = refreshed.is_ok(); refreshed } @@ -436,14 +445,20 @@ impl Session { #[tracing::instrument( name = "capability_roots.snapshot_for_step", skip_all, - fields(root_count = ready_selected_capability_roots.len()) + fields(root_count = resolved_roots.len()) )] pub(crate) async fn executor_capability_discovery_for_step( &self, config: &Config, - ready_selected_capability_roots: &[SelectedCapabilityRoot], + resolved_roots: &[ResolvedSelectedCapabilityRoot], environments: &TurnEnvironmentSnapshot, - ) -> Option> { + ) -> (Option>, bool) { + if config + .features + .enabled(Feature::ExecutorCapabilityDiscoveryV2) + { + return (None, false); + } let restricted_file_system = environments.primary().map_or_else( || { !config @@ -465,7 +480,7 @@ impl Session { .features .enabled(Feature::ExecutorCapabilityDiscovery) { - return None; + return (None, false); } let sandbox_contexts = if restricted_file_system { environments @@ -480,14 +495,14 @@ impl Session { } else { HashMap::new() }; - let environment_manager = self.services.turn_environments.environment_manager(); let cache = self .services .thread_extension_data - .get_or_init(|| ExecutorCapabilityDiscoveryCache::new(environment_manager)); - let selected_capability_roots = ready_selected_capability_roots + .get_or_init(ExecutorCapabilityDiscoveryCache::default); + let selected_capability_roots = resolved_roots .iter() - .filter(|selected_root| { + .filter(|root| { + let selected_root = root.selected_root(); if !restricted_file_system { return true; } @@ -507,24 +522,20 @@ impl Session { let discovery = cache .snapshot(&selected_capability_roots, &sandbox_contexts) .await; - if cache.take_recovered_discovery() { - // Root selection is unchanged, but recovered manifests can change MCP servers. - self.mark_mcp_runtime_dirty(); - } - Some(Arc::new(discovery)) + (Some(Arc::new(discovery)), cache.take_recovered_discovery()) } - /// Acquires V2 inventories from the thread's current cache. + /// Acquires V2 inventories and reports changes without scheduling an MCP refresh. pub(super) async fn executor_capability_discovery_for_step_v2( &self, config: &Config, environments: &TurnEnvironmentSnapshot, - ) -> Option> { + ) -> (Option>, bool) { if !config .features .enabled(Feature::ExecutorCapabilityDiscoveryV2) { - return None; + return (None, false); } // Retain this cache if a new turn replaces the thread's current cache. let cache = self @@ -567,11 +578,19 @@ impl Session { result, }); } - if cache.take_recovered_discovery() { - // Refresh MCP after discovery recovers. - self.mark_mcp_runtime_dirty(); - } - Some(snapshots) + let previous = self + .services + .thread_extension_data + .insert(snapshots.clone()); + let inventory_changed = previous.as_ref().map_or(!snapshots.is_empty(), |previous| { + previous.len() != snapshots.len() + || previous + .iter() + .zip(&snapshots) + .any(|(previous, current)| !previous.same_discovery(current)) + }); + let inventory_changed = cache.take_recovered_discovery() || inventory_changed; + (Some(snapshots), inventory_changed) } pub(crate) async fn resolve_selected_capability_roots_for_step( @@ -769,7 +788,6 @@ impl Session { error!("MCP runtime refresh semaphore closed"); return; }; - let auth = self.services.auth_manager.auth().await; { let mut state = self.state.lock().await; let mut config = (*state.session_configuration.original_config_do_not_use).clone(); @@ -783,20 +801,45 @@ impl Session { } state.session_configuration.original_config_do_not_use = Arc::new(config); } + // Claim after config mutation, before capturing the state this forced refresh handles. + self.mcp_refresh.claim(); + let mut refresh_invalidation = McpRefreshInvalidationGuard { + refresh: &self.mcp_refresh, + published: false, + }; + let auth = self.services.auth_manager.auth().await; let ready_selected_capability_roots = self .services .mcp_runtime .current_ready_selected_capability_roots(); let environments = self.services.turn_environments.snapshot().await; + // Adding MCP dependencies preserves the published root selection. + let mut captured_environments = environments.captured_environments(); + for root in &ready_selected_capability_roots { + let CapabilityRootLocation::Environment { environment_id, .. } = &root.location; + // A previously published root must not reconnect an environment absent from this capture. + captured_environments + .entry(environment_id.clone()) + .or_insert(None); + } + let selected_capability_roots = self + .services + .turn_environments + .environment_manager() + .resolve_selected_capability_roots( + &ready_selected_capability_roots, + &captured_environments, + ) + .await; let environment_selections = environments.all_selections(); let desired = self.latest_mcp_desired_state(auth, environments).await; - let executor_capability_discovery_v2 = self + let (executor_capability_discovery_v2, _) = self .executor_capability_discovery_for_step_v2(&desired.config, &desired.environments) .await; - let executor_capability_discovery = self + let (executor_capability_discovery, _) = self .executor_capability_discovery_for_step( &desired.config, - &ready_selected_capability_roots, + &selected_capability_roots, &desired.environments, ) .await; @@ -829,6 +872,7 @@ impl Session { elicitation_reviewer, ) .await; + refresh_invalidation.published = true; } pub(crate) fn ready_selected_capability_roots( diff --git a/codex-rs/core/src/session/mcp_refresh.rs b/codex-rs/core/src/session/mcp_refresh.rs index 9afd414bf6..f70a11d157 100644 --- a/codex-rs/core/src/session/mcp_refresh.rs +++ b/codex-rs/core/src/session/mcp_refresh.rs @@ -54,3 +54,7 @@ impl Drop for McpRefreshInvalidationGuard<'_> { } } } + +#[cfg(test)] +#[path = "mcp_refresh_tests.rs"] +mod tests; diff --git a/codex-rs/core/src/session/mcp_refresh_tests.rs b/codex-rs/core/src/session/mcp_refresh_tests.rs new file mode 100644 index 0000000000..f6a9d5dd23 --- /dev/null +++ b/codex-rs/core/src/session/mcp_refresh_tests.rs @@ -0,0 +1,27 @@ +//! Checks that successful refreshes preserve newly pending work. + +use super::*; +use pretty_assertions::assert_eq; + +#[test] +fn successful_manual_claim_consumes_only_preexisting_work() { + for initially_pending in [false, true] { + for invalidated_during_refresh in [false, true] { + let refresh = McpRefresh::new(); + if initially_pending { + refresh.invalidate(); + } + assert_eq!(refresh.claim(), initially_pending); + let mut guard = McpRefreshInvalidationGuard { + refresh: &refresh, + published: false, + }; + if invalidated_during_refresh { + refresh.invalidate(); + } + guard.published = true; + drop(guard); + assert_eq!(refresh.is_pending(), invalidated_during_refresh); + } + } +} diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index 3e5995eb53..c3b61304d8 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -3928,16 +3928,23 @@ impl Session { .await; let ready_selected_capability_roots = Self::ready_selected_capability_roots(&selected_capability_roots); - let executor_capability_discovery_v2 = self + let (executor_capability_discovery_v2, inventory_changed) = self .executor_capability_discovery_for_step_v2(&turn_context.config, &environments) .await; - let executor_capability_discovery = self - .executor_capability_discovery_for_step( + let v2_snapshots = executor_capability_discovery_v2.as_ref(); + let (executor_capability_discovery, discovery_recovered) = if v2_snapshots.is_some() { + (None, false) + } else { + self.executor_capability_discovery_for_step( &turn_context.config, - &ready_selected_capability_roots, + &selected_capability_roots, &environments, ) - .await; + .await + }; + if inventory_changed || discovery_recovered { + self.mark_mcp_runtime_dirty(); + } let extension_data = codex_extension_api::ExtensionData::new(turn_context.sub_id.clone()); if let Some(snapshots) = &executor_capability_discovery_v2 { @@ -4034,10 +4041,10 @@ impl Session { token_budget, session_telemetry, turn: turn_context, + extension_data, environments, selected_capability_roots, executor_capability_discovery, - extension_data, executor_capability_discovery_v2, mcp, tool_router, diff --git a/codex-rs/core/src/session/tests.rs b/codex-rs/core/src/session/tests.rs index 5793e8bba5..a9739fe2cb 100644 --- a/codex-rs/core/src/session/tests.rs +++ b/codex-rs/core/src/session/tests.rs @@ -10112,14 +10112,14 @@ async fn capability_discovery_uses_environment_permission_profile() { let environment_id = environment.selection.environment_id.clone(); turn_context.initial_environments.environments[0] = TurnEnvironmentState::Ready(environment); - let discovery = session + let (discovery, _) = session .executor_capability_discovery_for_step( &turn_context.config, - /*ready_selected_capability_roots*/ &[], + /*resolved_roots*/ &[], &turn_context.initial_environments, ) - .await - .expect("restricted environment should trigger capability discovery"); + .await; + let discovery = discovery.expect("restricted environment should trigger capability discovery"); assert_eq!( discovery.sandbox_contexts().get(&environment_id), diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index c9720799e2..7333c9060c 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -546,8 +546,7 @@ pub(crate) async fn run_turn( // Isolated Guardian reviewers deliberately have no skill catalog. if !crate::guardian::is_basic_session_source(&turn_context.session_source) { validate_required_skills( - &sess.services.thread_extension_data, - &turn_context.extension_data, + &step_context.extension_data, step_context.environments.required_skills(), ) .map_err(CodexErr::Fatal)?; @@ -1273,6 +1272,7 @@ async fn build_extension_turn_input_items( .collect::>(); let input = TurnInputContext { + step_store: &step_context.extension_data, turn_id: turn_context.sub_id.to_string(), user_input: user_input.to_vec(), environments, diff --git a/codex-rs/core/src/skills.rs b/codex-rs/core/src/skills.rs index 43ca328cf6..b68b3aab19 100644 --- a/codex-rs/core/src/skills.rs +++ b/codex-rs/core/src/skills.rs @@ -167,14 +167,16 @@ pub(crate) async fn emit_explicit_skill_invocations( pub(crate) async fn maybe_emit_implicit_skill_invocation( sess: &Session, - turn_context: &TurnContext, + step_context: &crate::session::step_context::StepContext, command: &str, workdir: &PathUri, native_workdir: Option<&AbsolutePathBuf>, environment_id: &str, ) { + let turn_context = step_context.turn.as_ref(); let Some(invocation) = detect_implicit_skill_invocation( turn_context.extension_data.as_ref(), + &step_context.extension_data, environment_id, command, workdir, diff --git a/codex-rs/core/src/thread_manager_tests.rs b/codex-rs/core/src/thread_manager_tests.rs index 533dfa44d3..5091ba987c 100644 --- a/codex-rs/core/src/thread_manager_tests.rs +++ b/codex-rs/core/src/thread_manager_tests.rs @@ -1959,6 +1959,42 @@ async fn start_thread_seeds_extension_data_for_mcp_and_lifecycle_contributors() mcp_loaded.load(Ordering::Relaxed) - loaded_before, expected_mcp_loads ); + // Installation-derived rejections must not become global plugin-ID denials. + let ready_roots = vec![SelectedCapabilityRoot { + id: "selected-a".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "env-a".to_string(), + path: PathUri::parse("file:///plugins/selected-a").expect("plugin root URI"), + }, + }]; + let selected = first_session + .services + .mcp_manager + .selected_plugins_for_step( + codex_extension_api::McpServerContributionContext::for_step( + &config, + &first_session.services.mcp_thread_init, + &first_session.services.thread_extension_data, + &first_originator, + &ready_roots, + /*executor_capability_discovery*/ None, + /*executor_capability_discovery_v2*/ None, + ) + .with_selected_environments(&[]) + .with_session_source(&SessionSource::Exec), + &config.plugins, + &disabled_plugin_ids, + ) + .await; + let mut selected_disabled_ids = selected.disabled_plugin_ids; + selected_disabled_ids.sort(); + selected_disabled_ids.dedup(); + assert_eq!(selected_disabled_ids, disabled_plugin_ids); + assert_eq!( + selected.disabled_plugin_roots, + projection.selected_plugins.disabled_plugin_roots + ); + assert_eq!(selected.plugins.len(), usize::from(selected_enabled)); } } diff --git a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs index ba75b1ed98..ce0a502453 100644 --- a/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs +++ b/codex-rs/core/src/tools/handlers/unified_exec/exec_command.rs @@ -242,7 +242,7 @@ impl ExecCommandHandler { let hook_command = args.cmd.clone(); maybe_emit_implicit_skill_invocation( session.as_ref(), - context.step_context.turn.as_ref(), + context.step_context.as_ref(), &hook_command, &cwd, native_cwd.as_ref(), diff --git a/codex-rs/core/tests/suite/executor_discovery_v2.rs b/codex-rs/core/tests/suite/executor_discovery_v2.rs index 7d3ce5e77b..c6572239d1 100644 --- a/codex-rs/core/tests/suite/executor_discovery_v2.rs +++ b/codex-rs/core/tests/suite/executor_discovery_v2.rs @@ -1,10 +1,12 @@ -//! Verifies gated, inert V2 acquisition and cache reuse across Core turns and idle callers. +//! Verifies V2 discovery gating, turn integration, and cache behavior. use anyhow::Context; use anyhow::Result; use codex_core::config::Config; use codex_core::windows_sandbox::WindowsSandboxLevelExt; use codex_core_plugins::PluginIdentity; +use codex_exec_server::DiscoverV2CapabilitiesResponse; +use codex_exec_server::ExecutorCapabilityDiscoveryV2Cache; use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; use codex_exec_server::HttpRequestBodyDeltaNotification; use codex_exec_server::HttpRequestParams; @@ -39,6 +41,7 @@ use tokio_util::task::AbortOnDropHandle; struct DiscoveryRecorder { snapshots: Arc>>, selected_plugins: Arc>>, + mcp_snapshots_per_step: Arc>>, } impl McpServerContributor for DiscoveryRecorder { @@ -48,9 +51,12 @@ impl McpServerContributor for DiscoveryRecorder { fn contribute<'a>( &'a self, - _context: McpServerContributionContext<'a, Config>, + context: McpServerContributionContext<'a, Config>, ) -> ExtensionFuture<'a, Vec> { - Box::pin(async { + Box::pin(async move { + if let Some(snapshots) = context.executor_capability_discovery_v2() { + self.snapshots.lock().await.extend_from_slice(snapshots); + } vec![McpServerContribution::HostedPluginConnectors { plugin_id: "plugin_sentinel".to_string(), plugin_display_name: "Inert sentinel".to_string(), @@ -73,8 +79,9 @@ impl McpServerContributor for DiscoveryRecorder { let selected_roots = context .ready_selected_capability_roots() .unwrap_or_default(); + // Core must reject configured legacy roots when V2 supplies the inventory. package_roots.extend(selected_roots.iter().map(|root| root.id.clone())); - if !selected_roots.is_empty() { + if !selected_roots.is_empty() && context.executor_capability_discovery_v2().is_none() { let legacy = context.executor_capability_discovery(); assert_eq!( legacy.map(|snapshot| snapshot.roots().len()), @@ -85,7 +92,6 @@ impl McpServerContributor for DiscoveryRecorder { })); } if let Some(snapshots) = context.executor_capability_discovery_v2() { - self.snapshots.lock().await.extend_from_slice(snapshots); for snapshot in snapshots { if let Ok(inventory) = &snapshot.result { package_roots.extend( @@ -127,6 +133,11 @@ impl ContextContributor for DiscoveryRecorder { input: WorldStateContributionInput<'a>, ) -> ExtensionFuture<'a, Vec> { Box::pin(async move { + let snapshot_count = self.snapshots.lock().await.len(); + self.mcp_snapshots_per_step + .lock() + .await + .push(snapshot_count); if let Some(snapshot) = input.step_store.get::() { *self.selected_plugins.lock().await = Some(snapshot.as_ref().clone()); } @@ -223,10 +234,16 @@ impl Executor { "mcpConfig":{"path":"file:///inert-sentinel/.codex-plugin/plugin.json", "contents":"{\"inert-sentinel\":{\"command\":\"never-run\"}}"}, "appsConfig":null},"skills":[]}], - "skills":[{"name":"inert-sentinel","description":"must not enter the prompt", + "skills":[{"name":"inert-sentinel","description":"Executor skill used by the acquisition test", "shortDescription":null,"path":"file:///inert-sentinel/SKILL.md","metadata":null}], "warnings":[] })), + Some("fs/open") if request["params"]["path"] == "file:///inert-sentinel/SKILL.md" => Some(json!({"handleId": request["params"]["handleId"]})), + Some("fs/readBlock") => Some(json!(codex_exec_server::FsReadBlockResponse { + chunk: b"# Sentinel instructions from the captured executor".to_vec().into(), + eof: true, + })), + Some("fs/close") => Some(json!({})), Some("fs/canonicalize") => Some(json!({"path":request["params"]["path"]})), Some("fs/walk") => Some(json!({"entries":[],"errors":[],"truncated":false})), _ => None, @@ -251,22 +268,150 @@ impl Executor { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn flag_off_never_requests_v2() -> Result<()> { +async fn v2_disabled_skips_inventory_discovery() -> Result<()> { check_acquisition(/*enabled*/ false, /*supported*/ true).await } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn v2_inventory_reaches_mcp_projection_without_loading_skills() -> Result<()> { +async fn v2_shares_inventory_with_mcp_and_skills_until_next_turn() -> Result<()> { check_acquisition(/*enabled*/ true, /*supported*/ true).await } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn unsupported_executor_keeps_existing_behavior() -> Result<()> { - check_acquisition(/*enabled*/ true, /*supported*/ false).await +async fn v2_changed_inventory_refreshes_mcp_without_selection_changes() -> Result<()> { + skip_if_no_network!(Ok(())); + + #[derive(Default)] + struct InventoryRecorder { + projections: AtomicUsize, + projected: Mutex>>, + steps: Mutex< + Vec<( + Arc, + Arc, + )>, + >, + } + + impl McpServerContributor for InventoryRecorder { + fn id(&self) -> &'static str { + "inventory-refresh-recorder" + } + + fn contribute<'a>( + &'a self, + context: McpServerContributionContext<'a, Config>, + ) -> ExtensionFuture<'a, Vec> { + Box::pin(async move { + self.projections.fetch_add(1, Ordering::SeqCst); + *self.projected.lock().await = context + .executor_capability_discovery_v2() + .and_then(|snapshots| snapshots.first()) + .and_then(|snapshot| snapshot.result.as_ref().ok()) + .cloned(); + Vec::new() + }) + } + } + + impl ContextContributor for InventoryRecorder { + fn contribute_world_state<'a>( + &'a self, + input: WorldStateContributionInput<'a>, + ) -> ExtensionFuture<'a, Vec> { + Box::pin(async move { + let inventory = input + .executor_capability_discovery_v2 + .and_then(|snapshots| snapshots.first()) + .and_then(|snapshot| snapshot.result.as_ref().ok()) + .expect("step V2 inventory") + .clone(); + let projected = self.projected.lock().await.clone().expect("MCP inventory"); + let mut steps = self.steps.lock().await; + steps.push((inventory, projected)); + // Model a fresh inventory after each step, without changing selected roots. + input + .thread_store + .insert(ExecutorCapabilityDiscoveryV2Cache::default()); + Vec::new() + }) + } + } + + let executor = Executor::start(/*supports_v2*/ true, DiscoveryFailure::None).await?; + let model = responses::start_mock_server().await; + let recorder = Arc::new(InventoryRecorder::default()); + let mut extensions = ExtensionRegistryBuilder::new(); + extensions.prompt_contributor(recorder.clone()); + extensions.mcp_server_contributor(recorder.clone()); + let test = test_codex() + .with_extensions(Arc::new(extensions.build())) + .with_exec_server_url(executor.url.clone()) + .with_config(|config| { + config.update_plan_enabled = true; + config + .features + .enable(Feature::ExecutorCapabilityDiscoveryV2) + .expect("enable discovery V2"); + }) + .build_with_auto_env(&model) + .await?; + let first = responses::mount_sse_once( + &model, + responses::sse(vec![ + responses::ev_response_created("first"), + responses::ev_function_call( + "plan-call", + "update_plan", + r#"{"plan":[{"step":"Say hello","status":"completed"}]}"#, + ), + responses::ev_completed("first"), + ]), + ) + .await; + let second = responses::mount_sse_once( + &model, + responses::sse(vec![ + responses::ev_response_created("second"), + responses::ev_completed("second"), + ]), + ) + .await; + test.submit_turn("Say hello.").await?; + first.single_request(); + second.single_request(); + { + let steps = recorder.steps.lock().await; + let (initial, _) = steps.first().context("first captured inventory")?; + assert!( + steps + .iter() + .any(|(inventory, _)| !Arc::ptr_eq(initial, inventory)), + "the next step must acquire a fresh inventory" + ); + for (inventory, projected) in steps.iter() { + assert!( + Arc::ptr_eq(inventory, projected), + "MCP must project the step's changed inventory even when selections are unchanged" + ); + } + } + let projections = recorder.projections.load(Ordering::SeqCst); + // Refresh before another step can discover the new inventory. This fixture has no Apps + // server, but the refresh must still publish exactly once before reporting that absence. + let error = test + .codex + .refresh_codex_apps_tools() + .await + .err() + .expect("no Apps server"); + assert!(error.to_string().contains("unknown MCP server")); + assert_eq!(recorder.projections.load(Ordering::SeqCst), projections + 1); + Ok(()) } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn discovery_failures_retry_or_remain_cached_by_error_kind() -> Result<()> { +async fn v2_discovery_failures_retry_or_remain_cached_by_error_kind() -> Result<()> { skip_if_no_network!(Ok(())); for failure in [DiscoveryFailure::Server, DiscoveryFailure::Disconnect] { let executor = Executor::start(/*supports_v2*/ true, failure).await?; @@ -344,13 +489,31 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { let model = responses::start_mock_server().await; let observed = Arc::new(Mutex::new(Vec::new())); let selected_plugins = Arc::new(Mutex::new(None)); + let mcp_snapshots_per_step = Arc::new(Mutex::new(Vec::new())); let mut extensions = ExtensionRegistryBuilder::new(); let recorder = Arc::new(DiscoveryRecorder { snapshots: Arc::clone(&observed), selected_plugins: Arc::clone(&selected_plugins), + mcp_snapshots_per_step: Arc::clone(&mcp_snapshots_per_step), }); extensions.prompt_contributor(recorder.clone()); extensions.mcp_server_contributor(recorder); + codex_skills_extension::install_with_providers( + &mut extensions, + codex_skills_extension::SkillProviders::new().with_executor_provider(Arc::new( + codex_skills_extension::ExecutorSkillProvider::new_with_restriction_product( + Arc::new(codex_exec_server::EnvironmentManager::default_for_tests()), + /*restriction_product*/ None, + ), + )), + |config: &Config| codex_skills_extension::SkillsExtensionConfig { + include_instructions: true, + max_context_tokens: config.skill_max_context_tokens, + bundled_skills_enabled: false, + cloud_skill_enabled: false, + shadow_selection_enabled: false, + }, + ); let test = test_codex() .with_extensions(Arc::new(extensions.build())) .with_exec_server_url(executor.url.clone()) @@ -386,6 +549,7 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { let acquires_v2 = enabled && supported; if acquires_v2 { observed.lock().await.clear(); + mcp_snapshots_per_step.lock().await.clear(); let _ = test.codex.current_mcp_config_and_runtime_context().await; let initial_calls = calls().await; let _ = test.codex.current_mcp_config_and_runtime_context().await; @@ -435,6 +599,7 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { let mut previous_inventory = None; for turn in 0..turns { observed.lock().await.clear(); + mcp_snapshots_per_step.lock().await.clear(); *selected_plugins.lock().await = None; let before_turn = calls().await; let mut bodies = Vec::new(); @@ -457,8 +622,27 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { for body in bodies { responses.push(responses::mount_sse_once(&model, body).await); } - test.submit_turn_with_environments("Say hello.", Some(vec![selection.clone()])) + let prompt = if acquires_v2 { + "Use $inert-sentinel to say hello." + } else { + "Say hello." + }; + test.submit_turn_with_environments(prompt, Some(vec![selection.clone()])) .await?; + if acquires_v2 { + let counts = mcp_snapshots_per_step.lock().await; + let first = counts + .first() + .context("MCP snapshot count for the first step")?; + assert!( + counts.len() >= 2, + "the turn must prepare multiple model steps" + ); + assert!( + counts.iter().all(|count| count == first), + "unchanged V2 read routes must not refresh MCP between steps: {counts:?}" + ); + } let executor_root = if acquires_v2 { format!("{}:inert-sentinel@market", selection.environment_id) } else { @@ -468,7 +652,7 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { plugin_id: "inert-sentinel@market".to_string(), }; // Both paths publish accepted Cloud packages and available installations only. - let expected_plugins = vec![ + let mut expected_plugins = vec![ (Some(executor_root), local_identity), ( None, @@ -477,9 +661,12 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { }, ), ]; + if enabled && !supported { + expected_plugins.retain(|(root, _)| root.is_none()); + } { let selected_plugins = selected_plugins.lock().await; - let snapshot = selected_plugins.as_ref().context("turn plugin snapshot")?; + let snapshot = selected_plugins.as_ref().context("step plugin snapshot")?; assert_eq!( snapshot .plugins @@ -496,7 +683,21 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { .collect::>(); assert_eq!(model_requests.len(), if acquires_v2 { 2 } else { 1 }); for request in model_requests { - assert!(!request.body_json().to_string().contains("inert-sentinel")); + if acquires_v2 { + assert!( + request + .body_json() + .to_string() + .contains("Sentinel instructions from the captured executor") + ); + } + let developer_text = request.message_input_texts("developer").join("\n"); + assert_eq!( + developer_text + .contains("- inert-sentinel: Executor skill used by the acquisition test"), + acquires_v2, + "only a successful V2 inventory renders the skill catalog", + ); } let turn_calls = calls().await; if acquires_v2 { @@ -535,12 +736,29 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { } } let requests = executor.requests.lock().await.clone(); - if !acquires_v2 { + assert_eq!( + requests + .iter() + .any(|request| request["method"] == "capabilityRoots/discoverV1"), + !enabled, + "the V2 flag selects one protocol, including when no executor supplies a V2 snapshot", + ); + if acquires_v2 { + let reads = requests + .iter() + .filter(|request| request["method"] == "fs/open") + .collect::>(); assert!( - requests - .iter() - .any(|request| request["method"] == "capabilityRoots/discoverV1") + !reads.is_empty(), + "explicit mentions must read through the captured executor" ); + for read in reads { + assert!(read["params"]["sandbox"].is_object()); + assert_eq!( + read["params"]["sandbox"]["policyContext"]["cwd"], + json!(cwd) + ); + } } let discovery = requests .iter() @@ -556,8 +774,8 @@ async fn check_acquisition(enabled: bool, supported: bool) -> Result<()> { } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] -async fn discovery_cache_coalesces_requests_and_separates_cwds_sandboxes_and_executors() --> Result<()> { +async fn v2_discovery_cache_reuses_matching_requests_and_isolates_execution_contexts() -> Result<()> +{ skip_if_no_network!(Ok(())); let root = tempfile::tempdir()?; let executor = Executor::start(/*supports_v2*/ true, DiscoveryFailure::None).await?; diff --git a/codex-rs/core/tests/suite/remote_env_required_skills_tests.rs b/codex-rs/core/tests/suite/remote_env_required_skills_tests.rs index 210d4fb4e6..bb520fd513 100644 --- a/codex-rs/core/tests/suite/remote_env_required_skills_tests.rs +++ b/codex-rs/core/tests/suite/remote_env_required_skills_tests.rs @@ -58,6 +58,7 @@ async fn required_environment_skill_gates_inference(availability: Availability) ); entry.enabled = !matches!(availability, Availability::Disabled); let catalog = SkillCatalog { + is_partial: false, entries: if matches!( availability, Availability::Missing | Availability::FailedEnvironment diff --git a/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs b/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs index 155b207161..977c7b7a42 100644 --- a/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs +++ b/codex-rs/core/tests/suite/scenarios_skill_catalog_dedup.rs @@ -64,6 +64,7 @@ async fn cloud_preference_preserves_executor_aliases_and_description_budget() -> let provider = |entries| { Arc::new(CatalogSkillProvider { catalog: SkillCatalog { + is_partial: false, entries, warnings: Vec::new(), }, diff --git a/codex-rs/core/tests/suite/skills_extension.rs b/codex-rs/core/tests/suite/skills_extension.rs index be8d77f167..0aef50aec2 100644 --- a/codex-rs/core/tests/suite/skills_extension.rs +++ b/codex-rs/core/tests/suite/skills_extension.rs @@ -316,6 +316,7 @@ const EXECUTOR_CATALOG: [(&str, &str); 6] = [ fn executor_catalog(skills: &[(&str, &str)]) -> SkillCatalog { SkillCatalog { + is_partial: false, entries: skills .iter() .map(|(name, description)| { @@ -935,6 +936,7 @@ async fn production_turn_aliases_executor_skill_roots() -> Result<()> { ) .await; let catalog = SkillCatalog { + is_partial: false, entries: ["search", "review", "summarize"] .into_iter() .map(|name| { @@ -1831,6 +1833,7 @@ async fn explicit_executor_skill_prompt_rejects_oversized_resource() -> Result<( ) .await?; let catalog = SkillCatalog { + is_partial: false, entries: vec![SkillCatalogEntry::new( SkillPackageId("skill://prompt-limit/live".to_string()), SkillAuthority::new(SkillSourceKind::Executor, "prompt-limit"), @@ -1948,6 +1951,7 @@ async fn executor_skill_invocation_is_environment_scoped_and_deduplicated() -> R std::fs::write(&skill_path, "executor skill contents\n")?; let skill_uri = PathUri::from_host_native_path(&skill_path)?; let catalog = SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId("skill://other-root/demo".to_string()), @@ -2057,6 +2061,7 @@ async fn production_turn_aliases_catalogs_with_separate_cloud_budget() -> Result let resource_catalog = |kind: SkillSourceKind, authority: &str, root: &str, prefix: &str| SkillCatalog { + is_partial: false, entries: ["search", "review", "summarize"] .into_iter() .map(|suffix| { @@ -2207,6 +2212,7 @@ async fn assert_catalog_model_switch(max_context_tokens: Option) -> Resul .expect("metrics provider"); let skill_count = 800; let catalog = SkillCatalog { + is_partial: false, entries: (0..skill_count) .map(|index| { let name = format!("skill-{index:03}"); @@ -2654,6 +2660,7 @@ async fn production_turn_uses_provider_host_catalog_and_core_snapshot_injection( let provider_description = "This skill comes from the extension host provider."; let provider_contents = "# Provider instructions that Core must not inject."; let provider_catalog = SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId(skill_resource.clone()), @@ -2768,6 +2775,7 @@ async fn production_turn_suppresses_only_the_superseded_host_skill_prompt() -> R let provider_resource = "skill://test/first-host/SKILL.md"; let provider_contents = "FIRST_PROVIDER_BODY"; let catalog = SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId("test/first-host".to_string()), @@ -3026,6 +3034,7 @@ async fn production_turn_keeps_rebalanced_catalogs_stable_after_compaction_and_r let skill_description = "Search available company knowledge. ".repeat(40); let skill_resource = "skill://codex_apps/cloud-search/SKILL.md"; let catalog = SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId("cloud/cloud-search".to_string()), @@ -3530,6 +3539,7 @@ async fn production_turn_fairly_shortens_extension_catalog_descriptions() -> Res let source_kind = SkillSourceKind::Custom("test".to_string()); let description = "x".repeat(1_025); let catalog = SkillCatalog { + is_partial: false, entries: (0..10) .map(|index| { let name = format!("skill-{index:02}"); diff --git a/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs b/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs index 2f86e1cafb..e377794a66 100644 --- a/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs +++ b/codex-rs/core/tests/suite/skills_extension/cloud_lifecycle_tests.rs @@ -41,6 +41,7 @@ impl SkillProvider for CloudTurnProvider { }) .collect(); Ok(SkillCatalog { + is_partial: false, entries, warnings: Vec::new(), }) diff --git a/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs b/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs index aa4f6d5aaa..c298c61091 100644 --- a/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs +++ b/codex-rs/core/tests/suite/skills_extension/cloud_skill_tests.rs @@ -123,6 +123,7 @@ text({ names: result.skills.map(skill => skill.name), warnings: result.warnings, &mut extensions, SkillProviders::new().with_cloud_provider(Arc::new(FakeCloudSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: [ "alpha-visible", "bravo-visible-skill", @@ -482,6 +483,7 @@ async fn production_turn_aliases_discovered_singleton_cloud_root() -> Result<()> &mut extensions, SkillProviders::new().with_cloud_provider(Arc::new(FakeCloudSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId(format!("{SKILL_ROOT}/search")), @@ -580,6 +582,7 @@ async fn cloud_skill_reads_run_in_parallel() -> Result<()> { &mut extensions, SkillProviders::new().with_cloud_provider(Arc::new(FakeCloudSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![SkillCatalogEntry::new( SkillPackageId(PACKAGE.to_string()), SkillAuthority::new(SkillSourceKind::Cloud, CODEX_APPS_MCP_SERVER_NAME), @@ -689,6 +692,7 @@ async fn cloud_skill_can_read_referenced_resource_without_an_executor() -> Resul let responses_server = responses::start_mock_server().await; let provider = Arc::new(FakeCloudSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId(SKILL_RESOURCE_URI.to_string()), diff --git a/codex-rs/core/tests/suite/skills_extension/yielded_skill_tests.rs b/codex-rs/core/tests/suite/skills_extension/yielded_skill_tests.rs index d207a9ca6a..e5753764af 100644 --- a/codex-rs/core/tests/suite/skills_extension/yielded_skill_tests.rs +++ b/codex-rs/core/tests/suite/skills_extension/yielded_skill_tests.rs @@ -17,6 +17,7 @@ async fn yielded_skill_read_keeps_originating_turn_metadata() -> Result<()> { &mut extensions, SkillProviders::new().with_cloud_provider(Arc::new(FakeCloudSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![SkillCatalogEntry::new( SkillPackageId(PACKAGE.to_string()), SkillAuthority::new(SkillSourceKind::Cloud, CODEX_APPS_MCP_SERVER_NAME), diff --git a/codex-rs/exec-server-protocol/src/capabilities.rs b/codex-rs/exec-server-protocol/src/capabilities.rs index 3e952f318b..29b638796a 100644 --- a/codex-rs/exec-server-protocol/src/capabilities.rs +++ b/codex-rs/exec-server-protocol/src/capabilities.rs @@ -20,6 +20,18 @@ pub struct ExecutorCapabilityDiscoveryV2Snapshot { pub result: Result, String>, } +impl ExecutorCapabilityDiscoveryV2Snapshot { + /// Compares discovery identity, not inventory contents; repeated errors compare by message. + pub fn same_discovery(&self, other: &Self) -> bool { + self.environment_id == other.environment_id + && match (&self.result, &other.result) { + (Ok(previous), Ok(current)) => Arc::ptr_eq(previous, current), + (Err(previous), Err(current)) => previous == current, + (Ok(_), Err(_)) | (Err(_), Ok(_)) => false, + } + } +} + /// Discover installed capabilities for a thread, without client-supplied installation paths. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "camelCase")] diff --git a/codex-rs/exec-server-protocol/src/capabilities_tests.rs b/codex-rs/exec-server-protocol/src/capabilities_tests.rs index eda7263af5..7951dea723 100644 --- a/codex-rs/exec-server-protocol/src/capabilities_tests.rs +++ b/codex-rs/exec-server-protocol/src/capabilities_tests.rs @@ -1,4 +1,4 @@ -//! V2 discovery requests and responses round-trip through JSON. +//! V2 discovery wire contracts. use super::*; use crate::CapabilityTextFile; diff --git a/codex-rs/exec-server/src/capability_discovery_cache.rs b/codex-rs/exec-server/src/capability_discovery_cache.rs index 94973251f1..ecc8c4b323 100644 --- a/codex-rs/exec-server/src/capability_discovery_cache.rs +++ b/codex-rs/exec-server/src/capability_discovery_cache.rs @@ -1,6 +1,9 @@ +//! Caches root discovery by captured executor/session, root, and sandbox. + use std::collections::BTreeMap; use std::collections::HashMap; use std::sync::Arc; +use std::sync::Weak; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; @@ -11,17 +14,18 @@ use tokio::sync::Mutex; use crate::CapabilityRootDiscoverRequest; use crate::CapabilityRootDiscovery; use crate::CapabilityRootsDiscoverParams; -use crate::EnvironmentManager; +use crate::Environment; use crate::ExecutorCapabilityDiscoverySnapshot; use crate::FileSystemSandboxContext; +use crate::ResolvedSelectedCapabilityRoot; /// Thread-scoped cache shared by capability consumers using the high-level executor API. /// /// A single miss batches every requested root by environment. Successful discoveries and -/// permanent failures remain cached by root and sandbox; transient failures are retried on the +/// permanent failures remain cached by executor/session, root, and sandbox; transient failures are retried on the /// next request. Recovery is reported so dependent MCP projections can be invalidated. +#[derive(Default)] pub struct ExecutorCapabilityDiscoveryCache { - environment_manager: Arc, entries: Mutex>, recovered_discovery: AtomicBool, } @@ -34,64 +38,136 @@ impl std::fmt::Debug for ExecutorCapabilityDiscoveryCache { } } -struct CachedRoot { +#[derive(Clone, Debug)] +struct RootDiscoveryKey { selected_root: SelectedCapabilityRoot, sandbox: Option, + environment: Weak, + session_id: Option, +} + +impl PartialEq for RootDiscoveryKey { + fn eq(&self, other: &Self) -> bool { + self.selected_root == other.selected_root + && self.sandbox == other.sandbox + && self.environment.ptr_eq(&other.environment) + && self.session_id == other.session_id + } +} + +#[derive(Clone)] +struct RootDiscoveryRequest { + key: RootDiscoveryKey, + environment: Result, String>, +} + +struct CachedRoot { + key: RootDiscoveryKey, result: Result, String>, // Preserve transport classification after the public snapshot reduces errors to strings. retryable: bool, } impl ExecutorCapabilityDiscoveryCache { - pub fn new(environment_manager: Arc) -> Self { - Self { - environment_manager, - entries: Mutex::new(Vec::new()), - recovered_discovery: AtomicBool::new(false), - } - } - /// Reports whether a previously failed root has recovered since the last observation. pub fn take_recovered_discovery(&self) -> bool { self.recovered_discovery.swap(false, Ordering::AcqRel) } - /// Returns discoveries in the same order as `selected_roots`. + /// Returns discoveries in root order using only the captured environment handles. #[tracing::instrument( name = "capability_roots.discovery_cache.resolve", skip_all, - fields(root_count = selected_roots.len()) + fields(root_count = roots.len()) )] pub async fn discover( &self, - selected_roots: &[SelectedCapabilityRoot], + roots: &[ResolvedSelectedCapabilityRoot], sandbox_contexts: &HashMap, ) -> Vec, String>> { - let missing = { + let environments = roots + .iter() + .map(|root| { + let CapabilityRootLocation::Environment { environment_id, .. } = + &root.selected_root().location; + (environment_id.clone(), Arc::clone(root.environment())) + }) + .collect::>(); + // Resolve a session once per captured environment, before matching cached results. + let bindings = futures::future::join_all(environments.into_iter().map( + |(id, environment)| async move { + let session = environment + .executor_session_id() + .await + .map_err(|error| error.to_string()); + (id, (environment, session)) + }, + )) + .await + .into_iter() + .collect::>(); + let requests = roots + .iter() + .map(|root| { + let selected_root = root.selected_root(); + let CapabilityRootLocation::Environment { environment_id, .. } = + &selected_root.location; + let (environment, session) = &bindings[environment_id]; + RootDiscoveryRequest { + key: RootDiscoveryKey { + selected_root: selected_root.clone(), + sandbox: sandbox_contexts.get(environment_id).cloned(), + environment: Arc::downgrade(environment), + session_id: session.as_ref().ok().cloned().flatten(), + }, + environment: session + .as_ref() + .map(|_| Arc::clone(environment)) + .map_err(Clone::clone), + } + }) + .collect::>(); + let keys = requests + .iter() + .map(|request| request.key.clone()) + .collect::>(); + let mut results = { let entries = self.entries.lock().await; - selected_roots + requests .iter() - .filter(|selected_root| { - let CapabilityRootLocation::Environment { environment_id, .. } = - &selected_root.location; - let sandbox = sandbox_contexts.get(environment_id); - !entries.iter().any(|cached| { - cached.selected_root == **selected_root - && cached.sandbox.as_ref() == sandbox - && !cached.retryable + .map(|request| { + request.environment.as_ref().ok().and_then(|_| { + entries + .iter() + .find(|cached| cached.key == request.key && !cached.retryable) + .map(|cached| cached.result.clone()) }) }) - .cloned() .collect::>() }; - let discovered = self.discover_missing(missing, sandbox_contexts).await; + let missing = requests + .into_iter() + .zip(&results) + .filter_map(|(request, result)| result.is_none().then_some(request)) + .collect(); + let discovered = self.discover_missing(missing).await; let mut entries = self.entries.lock().await; for discovered_root in discovered { + // Reuse a concurrent success only for the same captured binding. + let resolved = entries + .iter() + .find(|cached| cached.key == discovered_root.key && cached.result.is_ok()) + .map_or(&discovered_root.result, |cached| &cached.result); + for (key, result) in keys.iter().zip(&mut results) { + if *key == discovered_root.key && result.is_none() { + *result = Some(resolved.clone()); + } + } if let Some(cached) = entries .iter_mut() - .find(|cached| cached.selected_root == discovered_root.selected_root) + .find(|cached| cached.key.selected_root == discovered_root.key.selected_root) { - if cached.sandbox != discovered_root.sandbox || cached.result.is_err() { + if cached.key != discovered_root.key || cached.result.is_err() { if cached.result.is_err() && discovered_root.result.is_ok() { self.recovered_discovery.store(true, Ordering::Release); } @@ -101,146 +177,133 @@ impl ExecutorCapabilityDiscoveryCache { entries.push(discovered_root); } } - selected_roots - .iter() - .map(|selected_root| { - let CapabilityRootLocation::Environment { environment_id, .. } = - &selected_root.location; - let sandbox = sandbox_contexts.get(environment_id); - match entries.iter().find(|cached| { - cached.selected_root == *selected_root && cached.sandbox.as_ref() == sandbox - }) { - Some(cached) => cached.result.clone(), - None => Err(format!( + keys.into_iter() + .zip(results) + .map(|(key, result)| { + result.unwrap_or_else(|| { + Err(format!( "selected capability root `{}` was not discovered", - selected_root.id - )), - } + key.selected_root.id + )) + }) }) .collect() } - /// Resolves the selected roots once and freezes their results for one model step. + /// Freezes discoveries and sandbox contexts for the captured roots. pub async fn snapshot( &self, - selected_roots: &[SelectedCapabilityRoot], + roots: &[ResolvedSelectedCapabilityRoot], sandbox_contexts: &HashMap, ) -> ExecutorCapabilityDiscoverySnapshot { - ExecutorCapabilityDiscoverySnapshot::new( - selected_roots, - self.discover(selected_roots, sandbox_contexts).await, - sandbox_contexts.clone(), - ) + let selected_roots = roots + .iter() + .map(|root| root.selected_root().clone()) + .collect::>(); + let results = self.discover(roots, sandbox_contexts).await; + ExecutorCapabilityDiscoverySnapshot::new(&selected_roots, results, sandbox_contexts.clone()) } - async fn discover_missing( - &self, - missing: Vec, - sandbox_contexts: &HashMap, - ) -> Vec { - let mut grouped = BTreeMap::>::new(); + async fn discover_missing(&self, missing: Vec) -> Vec { + let mut grouped = BTreeMap::>::new(); for selected_root in missing { let CapabilityRootLocation::Environment { environment_id, .. } = - &selected_root.location; + &selected_root.key.selected_root.location; grouped .entry(environment_id.clone()) .or_default() .push(selected_root); } - let batches = grouped.into_iter().flat_map(|(environment_id, roots)| { + let batches = grouped.into_values().flat_map(|roots| { roots .chunks(crate::capability_discovery::MAX_ROOTS_PER_REQUEST) - .map(|batch| (environment_id.clone(), batch.to_vec())) + .map(<[RootDiscoveryRequest]>::to_vec) .collect::>() }); - let discoveries = - futures::future::join_all(batches.map(|(environment_id, selected_roots)| async move { - let sandbox = sandbox_contexts.get(&environment_id).cloned(); - let Some(environment) = self.environment_manager.get_environment(&environment_id) - else { - let error = format!("environment `{environment_id}` is unavailable"); + let discoveries = futures::future::join_all(batches.map(|selected_roots| async move { + let environment = match &selected_roots[0].environment { + Ok(environment) => Arc::clone(environment), + Err(error) => { + let error = error.clone(); return selected_roots .into_iter() - .map(|selected_root| CachedRoot { - selected_root, - sandbox: sandbox.clone(), + .map(|request| CachedRoot { + key: request.key, result: Err(error.clone()), retryable: true, }) .collect::>(); - }; - let params = CapabilityRootsDiscoverParams { - roots: selected_roots - .iter() - .map(|selected_root| { - let CapabilityRootLocation::Environment { path, .. } = - &selected_root.location; - CapabilityRootDiscoverRequest { - id: selected_root.id.clone(), - path: path.clone(), - sandbox: sandbox.clone(), - } - }) - .collect(), - }; - let response = match environment.discover_capability_roots(params).await { - Ok(response) => response, - Err(error) => { - let retryable = crate::client::can_retry_connection_attempt(&error); - let error = error.to_string(); - return selected_roots - .into_iter() - .map(|selected_root| CachedRoot { - selected_root, - sandbox: sandbox.clone(), - result: Err(error.clone()), - retryable, - }) - .collect(); - } - }; - if response.roots.len() != selected_roots.len() { - let error = format!( - "exec-server returned {} capability roots for {} requests", - response.roots.len(), - selected_roots.len() - ); + } + }; + let params = CapabilityRootsDiscoverParams { + roots: selected_roots + .iter() + .map(|request| { + let selected_root = &request.key.selected_root; + let CapabilityRootLocation::Environment { path, .. } = + &selected_root.location; + CapabilityRootDiscoverRequest { + id: selected_root.id.clone(), + path: path.clone(), + sandbox: request.key.sandbox.clone(), + } + }) + .collect(), + }; + let response = match environment.discover_capability_roots(params).await { + Ok(response) => response, + Err(error) => { + let retryable = crate::client::can_retry_connection_attempt(&error); + let error = error.to_string(); return selected_roots .into_iter() - .map(|selected_root| CachedRoot { - selected_root, - sandbox: sandbox.clone(), + .map(|request| CachedRoot { + key: request.key, result: Err(error.clone()), - retryable: false, + retryable, }) .collect(); } - selected_roots + }; + if response.roots.len() != selected_roots.len() { + let error = format!( + "exec-server returned {} capability roots for {} requests", + response.roots.len(), + selected_roots.len() + ); + return selected_roots .into_iter() - .zip(response.roots) - .map(|(selected_root, discovery)| { - let CapabilityRootLocation::Environment { path, .. } = - &selected_root.location; - let result = if discovery.id == selected_root.id && discovery.path == *path - { - Ok(Arc::new(discovery)) - } else { - Err(format!( - "exec-server returned mismatched capability root `{}` at {}", - discovery.id, discovery.path - )) - }; - CachedRoot { - selected_root, - sandbox: sandbox.clone(), - result, - retryable: false, - } + .map(|request| CachedRoot { + key: request.key, + result: Err(error.clone()), + retryable: false, }) - .collect() - })) - .await; + .collect(); + } + selected_roots + .into_iter() + .zip(response.roots) + .map(|(request, discovery)| { + let selected_root = &request.key.selected_root; + let CapabilityRootLocation::Environment { path, .. } = &selected_root.location; + let result = if discovery.id == selected_root.id && discovery.path == *path { + Ok(Arc::new(discovery)) + } else { + Err(format!( + "exec-server returned mismatched capability root `{}` at {}", + discovery.id, discovery.path + )) + }; + CachedRoot { + key: request.key, + result, + retryable: false, + } + }) + .collect() + })) + .await; discoveries.into_iter().flatten().collect() } } diff --git a/codex-rs/exec-server/src/discoverV2/capability_discoveries.rs b/codex-rs/exec-server/src/discoverV2/capability_discoveries.rs index 5a8e51b024..910c60ad5b 100644 --- a/codex-rs/exec-server/src/discoverV2/capability_discoveries.rs +++ b/codex-rs/exec-server/src/discoverV2/capability_discoveries.rs @@ -229,12 +229,15 @@ fn parse_root( .filter(|name| !name.is_empty()) .unwrap_or_else(|| "skill".to_string()) }); - let Ok(parsed) = parsed else { - warnings.push(format!( - "invalid skill metadata at {}", - skill.instructions.path - )); - continue; + let parsed = match parsed { + Ok(parsed) => parsed, + Err(error) => { + warnings.push(format!( + "invalid skill metadata at {}: {error}", + skill.instructions.path + )); + continue; + } }; let path = skill.instructions.path; let mut ancestor = path.parent(); diff --git a/codex-rs/exec-server/tests/environment.rs b/codex-rs/exec-server/tests/environment.rs index fbbb3d37f1..ffacfb6ccb 100644 --- a/codex-rs/exec-server/tests/environment.rs +++ b/codex-rs/exec-server/tests/environment.rs @@ -375,7 +375,7 @@ async fn capability_discovery_retries_executor_disconnect_within_same_request() .info() .await?; - let cache = Arc::new(ExecutorCapabilityDiscoveryCache::new(Arc::clone(&manager))); + let cache = Arc::new(ExecutorCapabilityDiscoveryCache::default()); let skill_root = tempfile::tempdir()?; let selected_roots = vec![SelectedCapabilityRoot { id: "recovering-skill".to_string(), @@ -385,9 +385,18 @@ async fn capability_discovery_retries_executor_disconnect_within_same_request() }, }]; + let resolved_roots = manager + .resolve_selected_capability_roots( + &selected_roots, + &HashMap::from([( + REMOTE_ENVIRONMENT_ID.to_string(), + manager.get_environment(REMOTE_ENVIRONMENT_ID), + )]), + ) + .await; let snapshot = timeout( Duration::from_secs(12), - cache.snapshot(&selected_roots, &HashMap::new()), + cache.snapshot(&resolved_roots, &HashMap::new()), ) .await .context("capability discovery did not retry within the same request")?; @@ -409,7 +418,7 @@ async fn capability_discovery_retries_executor_disconnect_within_same_request() async fn capability_discovery_retries_after_executor_reconnects() -> anyhow::Result<()> { let server = exec_server().await?; let manager = Arc::new(EnvironmentManager::default_for_tests()); - let cache = ExecutorCapabilityDiscoveryCache::new(Arc::clone(&manager)); + let cache = ExecutorCapabilityDiscoveryCache::default(); let skill_root = tempfile::tempdir()?; let refused_listener = TcpListener::bind("127.0.0.1:0").await?; let refused_address = refused_listener.local_addr()?; @@ -427,7 +436,16 @@ async fn capability_discovery_retries_after_executor_reconnects() -> anyhow::Res }, }]; - let failed_snapshot = cache.snapshot(&selected_roots, &HashMap::new()).await; + let resolved_roots = manager + .resolve_selected_capability_roots( + &selected_roots, + &HashMap::from([( + "recovering".to_string(), + manager.get_environment("recovering"), + )]), + ) + .await; + let failed_snapshot = cache.snapshot(&resolved_roots, &HashMap::new()).await; assert!(failed_snapshot.roots()[0].result.is_err()); assert!(!cache.take_recovered_discovery()); @@ -442,7 +460,16 @@ async fn capability_discovery_retries_after_executor_reconnects() -> anyhow::Res .wait_until_ready() .await?; - let recovered_snapshot = cache.snapshot(&selected_roots, &HashMap::new()).await; + let resolved_roots = manager + .resolve_selected_capability_roots( + &selected_roots, + &HashMap::from([( + "recovering".to_string(), + manager.get_environment("recovering"), + )]), + ) + .await; + let recovered_snapshot = cache.snapshot(&resolved_roots, &HashMap::new()).await; let discovery = recovered_snapshot.roots()[0] .result .as_ref() @@ -469,7 +496,7 @@ async fn capability_discovery_retries_after_application_network_policy_recovers( server.websocket_url().to_string(), /*connect_timeout*/ None, )?; - let cache = ExecutorCapabilityDiscoveryCache::new(manager); + let cache = ExecutorCapabilityDiscoveryCache::default(); let skill_root = tempfile::tempdir()?; let selected_roots = vec![SelectedCapabilityRoot { id: "recovering-skill".to_string(), @@ -479,7 +506,16 @@ async fn capability_discovery_retries_after_application_network_policy_recovers( }, }]; - let failed = cache.snapshot(&selected_roots, &HashMap::new()).await; + let resolved_roots = manager + .resolve_selected_capability_roots( + &selected_roots, + &HashMap::from([( + "recovering".to_string(), + manager.get_environment("recovering"), + )]), + ) + .await; + let failed = cache.snapshot(&resolved_roots, &HashMap::new()).await; let error = failed.roots()[0].result.as_ref().unwrap_err(); assert!(error.contains("application network policy"), "{error}"); assert!(!cache.take_recovered_discovery()); @@ -490,7 +526,7 @@ async fn capability_discovery_retries_after_application_network_policy_recovers( ); let recovered = timeout( Duration::from_secs(/*secs*/ 10), - cache.snapshot(&selected_roots, &HashMap::new()), + cache.snapshot(&resolved_roots, &HashMap::new()), ) .await?; let discovery = recovered.roots()[0] @@ -502,3 +538,56 @@ async fn capability_discovery_retries_after_application_network_policy_recovers( assert!(!cache.take_recovered_discovery()); Ok(()) } + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn capability_discovery_does_not_replace_an_unavailable_captured_executor() +-> anyhow::Result<()> { + let server = exec_server().await?; + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let listener = TcpListener::bind("127.0.0.1:0").await?; + let address = listener.local_addr()?; + drop(listener); + manager.upsert_environment( + "captured".into(), + format!("ws://{address}"), + Some(Duration::from_millis(100)), + )?; + let original = manager.get_environment("captured").context("original")?; + let root_dir = tempfile::tempdir()?; + let roots = vec![SelectedCapabilityRoot { + id: "root".into(), + location: CapabilityRootLocation::Environment { + environment_id: "captured".into(), + path: PathUri::from_host_native_path(root_dir.path())?, + }, + }]; + // Model a handle retained by a step whose connection is no longer available. + let resolved = manager + .resolve_selected_capability_roots( + &roots, + &HashMap::from([("captured".into(), Some(original))]), + ) + .await; + manager.upsert_environment( + "captured".into(), + server.websocket_url().to_string(), + /*connect_timeout*/ None, + )?; + let replacement = manager.get_environment("captured").context("replacement")?; + replacement.wait_until_ready().await?; + let cache = ExecutorCapabilityDiscoveryCache::default(); + let unavailable = cache.snapshot(&resolved, &HashMap::new()).await; + assert!(unavailable.roots()[0].result.is_err()); + let newer = manager + .resolve_selected_capability_roots( + &roots, + &HashMap::from([("captured".into(), Some(replacement))]), + ) + .await; + let available = cache.snapshot(&newer, &HashMap::new()).await; + assert!(available.roots()[0].result.is_ok()); + assert!(cache.take_recovered_discovery()); + let unavailable = cache.snapshot(&resolved, &HashMap::new()).await; + assert!(unavailable.roots()[0].result.is_err()); + Ok(()) +} diff --git a/codex-rs/ext/extension-api/src/contributors.rs b/codex-rs/ext/extension-api/src/contributors.rs index 10947856fc..dfff65d5de 100644 --- a/codex-rs/ext/extension-api/src/contributors.rs +++ b/codex-rs/ext/extension-api/src/contributors.rs @@ -352,11 +352,14 @@ pub trait SkillInvocationContributor: Send + Sync { /// Extension contribution that exposes native tools owned by a feature. pub trait ToolContributor: Send + Sync { /// Returns native tools bound to the supplied extension state. + /// Step-only contributors can leave this empty default unchanged. fn tools( &self, - session_store: &ExtensionData, - thread_store: &ExtensionData, - ) -> Vec ToolExecutor>>>; + _session_store: &ExtensionData, + _thread_store: &ExtensionData, + ) -> Vec ToolExecutor>>> { + Vec::new() + } /// Returns native tools bound to one sampling step. fn tools_for_step( diff --git a/codex-rs/ext/extension-api/src/contributors/turn_input.rs b/codex-rs/ext/extension-api/src/contributors/turn_input.rs index 6375242ac9..7e5e168184 100644 --- a/codex-rs/ext/extension-api/src/contributors/turn_input.rs +++ b/codex-rs/ext/extension-api/src/contributors/turn_input.rs @@ -19,6 +19,8 @@ pub struct TurnInputEnvironment<'a> { /// Turn facts supplied before the host records turn-local model input items. #[derive(Clone)] pub struct TurnInputContext<'a> { + /// Prepared extension snapshots and read routes for the step processing this input. + pub step_store: &'a crate::ExtensionData, /// Stable host-owned turn identifier. pub turn_id: String, /// User input submitted for this turn. diff --git a/codex-rs/ext/mcp/src/cloud_plugin_tests.rs b/codex-rs/ext/mcp/src/cloud_plugin_tests.rs index 4fe13895b8..85496cf0bd 100644 --- a/codex-rs/ext/mcp/src/cloud_plugin_tests.rs +++ b/codex-rs/ext/mcp/src/cloud_plugin_tests.rs @@ -491,14 +491,14 @@ async fn run_cloud_catalog_refresh_lifecycle() -> anyhow::Result<()> { ( "partial-empty-refresh", Ok(partial_empty_catalog), - Some("plugin_remote"), + None, None, None, ), ( "same-account-failure", Err("temporarily unavailable".to_string()), - Some("plugin_remote"), + None, None, None, ), diff --git a/codex-rs/ext/mcp/src/plugin_contributor_state.rs b/codex-rs/ext/mcp/src/plugin_contributor_state.rs index c14802037d..fadf0a1d9f 100644 --- a/codex-rs/ext/mcp/src/plugin_contributor_state.rs +++ b/codex-rs/ext/mcp/src/plugin_contributor_state.rs @@ -125,10 +125,12 @@ impl PluginContributorState { auth_cache_key } + /// Accepts current-auth results, retaining prior complete entries as partial after an incomplete refresh. + /// Returns false only when the result belongs to a stale auth generation. pub(crate) fn publish_cloud_catalog( &mut self, auth_cache_key: Option<&McpResourceClientAuthKey>, - catalog: PluginCatalog, + mut catalog: PluginCatalog, ) -> bool { let Some(generation) = self .cloud_generation @@ -139,12 +141,16 @@ impl PluginContributorState { return false; }; if catalog.is_partial - && generation + && let Some(previous) = generation .catalog .as_ref() - .is_some_and(|previous| !previous.is_partial) + .filter(|previous| !previous.is_partial) { - return true; + catalog = previous.clone(); + catalog.is_partial = true; + tracing::warn!( + "cloud plugin discovery was partial; retaining prior auth-scoped entries as partial" + ); } generation.catalog = Some(catalog); true diff --git a/codex-rs/ext/mcp/src/plugin_contributor_state_tests.rs b/codex-rs/ext/mcp/src/plugin_contributor_state_tests.rs index ed4ed918c9..39f2ade974 100644 --- a/codex-rs/ext/mcp/src/plugin_contributor_state_tests.rs +++ b/codex-rs/ext/mcp/src/plugin_contributor_state_tests.rs @@ -107,6 +107,39 @@ fn executor_snapshot( }) } +#[test] +fn partial_refresh_retains_entries_without_excluding_new_executor_plugins() -> anyhow::Result<()> { + let mut state = PluginContributorState::default(); + let auth_key = state.prepare_cloud_generation(/*mcp_resources*/ None); + let original = PluginCatalog { + entries: vec![cloud_plugin("old_plugin")], + ..Default::default() + }; + assert!(state.publish_cloud_catalog(auth_key.as_ref(), original.clone())); + assert!(state.publish_cloud_catalog( + auth_key.as_ref(), + PluginCatalog { + entries: vec![cloud_plugin("plugin_a")], + is_partial: true, + ..Default::default() + }, + )); + let mut expected = original; + expected.is_partial = true; + assert_eq!(state.cloud_catalog(), Some(&expected)); + + state.update_executor_catalog(&[executor_snapshot("executor-a")?]); + let merged = + codex_core_plugins::merge_catalogs(state.executor_catalog.as_ref(), state.cloud_catalog()); + assert!( + merged + .entries + .iter() + .any(|entry| { entry.id.as_str() == "plugin_a" && entry.enabled }) + ); + Ok(()) +} + #[test] fn executor_refresh_preserves_order_and_removes_stale_entries() -> anyhow::Result<()> { let mut state = PluginContributorState::default(); @@ -181,8 +214,17 @@ async fn projection_respects_discovery_mode_and_preserves_enabled_plugins() -> a path: PathUri::from_host_native_path(&root)?, }, }]; - let v1 = ExecutorCapabilityDiscoveryCache::new(Arc::clone(&environments)) - .snapshot(&roots, &Default::default()) + let resolved_roots = environments + .resolve_selected_capability_roots( + &roots, + &std::collections::HashMap::from([( + LOCAL_ENVIRONMENT_ID.to_string(), + environments.get_environment(LOCAL_ENVIRONMENT_ID), + )]), + ) + .await; + let v1 = ExecutorCapabilityDiscoveryCache::default() + .snapshot(&resolved_roots, &Default::default()) .await; let mut contributor = PluginContributor { providers: PluginProviders::new(Arc::new(ExecutorPluginProvider::new(environments))) @@ -348,7 +390,7 @@ async fn projection_respects_discovery_mode_and_preserves_enabled_plugins() -> a warnings: vec!["partial cloud listing".to_string()], ..Default::default() }), - false, + true, ), ( "disabled", @@ -379,7 +421,9 @@ async fn projection_respects_discovery_mode_and_preserves_enabled_plugins() -> a true, ), ] { - let cloud_available = catalog.is_some(); + let has_cloud_counterpart = catalog + .as_ref() + .is_some_and(|catalog| !catalog.entries.is_empty()); contributor.providers.cloud = if cloud_enabled { Some(Arc::new(CloudProvider)) } else { @@ -406,7 +450,7 @@ async fn projection_respects_discovery_mode_and_preserves_enabled_plugins() -> a ), ) .await; - let expected = if !cloud_enabled || !cloud_available { + let expected = if enabled && !has_cloud_counterpart { vec!["mcp:executor-only", "executor-a:example@marketplace"] } else if enabled { vec![ diff --git a/codex-rs/ext/mcp/tests/plugin_mcp.rs b/codex-rs/ext/mcp/tests/plugin_mcp.rs index 9123fe43eb..c399199717 100644 --- a/codex-rs/ext/mcp/tests/plugin_mcp.rs +++ b/codex-rs/ext/mcp/tests/plugin_mcp.rs @@ -653,9 +653,18 @@ async fn raw_selected_plugin_contributions( .features .enabled(Feature::ExecutorCapabilityDiscovery) { + let resolved_roots = environment_manager + .resolve_selected_capability_roots( + &selected_capability_roots, + &HashMap::from([( + LOCAL_ENVIRONMENT_ID.to_string(), + environment_manager.get_environment(LOCAL_ENVIRONMENT_ID), + )]), + ) + .await; Some( - ExecutorCapabilityDiscoveryCache::new(environment_manager) - .snapshot(&selected_capability_roots, &Default::default()) + ExecutorCapabilityDiscoveryCache::default() + .snapshot(&resolved_roots, &Default::default()) .await, ) } else { diff --git a/codex-rs/ext/skills/Cargo.toml b/codex-rs/ext/skills/Cargo.toml index 34a396aebb..fa194ca760 100644 --- a/codex-rs/ext/skills/Cargo.toml +++ b/codex-rs/ext/skills/Cargo.toml @@ -39,6 +39,7 @@ tracing = { workspace = true } [dev-dependencies] assert_matches = { workspace = true } codex-attachment-store = { workspace = true } +codex-core-plugin-common = { workspace = true } codex-models-manager = { workspace = true } codex-protocol-test-support = { workspace = true } codex-utils-cargo-bin = { workspace = true } diff --git a/codex-rs/ext/skills/src/catalog.rs b/codex-rs/ext/skills/src/catalog.rs index 5783a7039d..e8e0aef88a 100644 --- a/codex-rs/ext/skills/src/catalog.rs +++ b/codex-rs/ext/skills/src/catalog.rs @@ -1,3 +1,6 @@ +//! Skill catalog data and reconciliation, retaining source-local policy and read routes. + +use codex_extension_api::SelectedPluginSnapshot; use codex_protocol::protocol::SkillScope; use codex_skills::SkillDependencies; use codex_utils_path_uri::PathUri; @@ -127,6 +130,12 @@ impl SkillResourceId { &self.id } + pub fn environment_id(&self) -> Option<&str> { + self.environment_path + .as_ref() + .map(|resource| resource.environment_id.as_str()) + } + pub(crate) fn bind_environment_package_resource( &self, package: &SkillPackageId, @@ -168,7 +177,7 @@ struct EnvironmentSkillResource { contents: Option>, } -/// Metadata shown in the always-visible skills catalog. +/// Skill metadata and source-owned read routes. #[derive(Clone, Debug, PartialEq, Eq)] pub struct SkillCatalogEntry { pub id: SkillPackageId, @@ -181,6 +190,10 @@ pub struct SkillCatalogEntry { pub canonical_skill_id: Option, pub plugin_id: Option, pub analytics_scope: Option, + /// Plugin-service identity for cross-source matching. + pub remote_plugin_id: Option, + /// Alternate read routes that retain their original authority. + pub sources: Vec, alias_root: Option, alias_root_order: Option, prompt_scope: Option, @@ -208,6 +221,8 @@ impl SkillCatalogEntry { canonical_skill_id: None, plugin_id: None, analytics_scope: None, + remote_plugin_id: None, + sources: Vec::new(), alias_root: None, alias_root_order: None, prompt_scope: None, @@ -217,6 +232,16 @@ impl SkillCatalogEntry { } } + pub fn with_remote_plugin_id(mut self, remote_plugin_id: Option) -> Self { + self.remote_plugin_id = remote_plugin_id; + self + } + + pub fn with_plugin_id(mut self, plugin_id: Option) -> Self { + self.plugin_id = plugin_id; + self + } + pub fn with_short_description(mut self, short_description: Option) -> Self { self.short_description = short_description; self @@ -262,6 +287,21 @@ impl SkillCatalogEntry { self.enabled && self.prompt_visible } + pub(crate) fn primary_source(&self) -> SkillSourceLocation { + SkillSourceLocation { + authority: self.authority.clone(), + package: self.id.clone(), + main_prompt: self.main_prompt.clone(), + display_path: self.display_path.clone(), + plugin_id: self.plugin_id.clone(), + enabled: self.enabled, + } + } + + pub(crate) fn source_locations(&self) -> impl Iterator + '_ { + std::iter::once(self.primary_source()).chain(self.sources.iter().cloned()) + } + pub(crate) fn rendered_path(&self) -> &str { self.display_path .as_deref() @@ -281,11 +321,24 @@ impl SkillCatalogEntry { } } +/// One original provider route retained by a reconciled skill. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct SkillSourceLocation { + pub authority: SkillAuthority, + pub package: SkillPackageId, + pub main_prompt: SkillResourceId, + pub display_path: Option, + /// Installation attribution belongs to the route used, not the merged primary entry. + pub plugin_id: Option, + pub enabled: bool, +} + /// Merged catalog for one turn. #[derive(Clone, Debug, Default, PartialEq, Eq)] pub struct SkillCatalog { pub entries: Vec, pub warnings: Vec, + pub is_partial: bool, } impl SkillCatalog { @@ -294,14 +347,15 @@ impl SkillCatalog { self.push_entry(entry); } self.warnings.extend(other.warnings); + self.is_partial |= other.is_partial; } pub fn push_entry(&mut self, entry: SkillCatalogEntry) { - if self - .entries - .iter() - .any(|existing| existing.authority == entry.authority && existing.id == entry.id) - { + if self.entries.iter().any(|existing| { + existing.authority == entry.authority + && existing.id == entry.id + && existing.main_prompt.environment_id() == entry.main_prompt.environment_id() + }) { return; } @@ -309,6 +363,100 @@ impl SkillCatalog { } } +/// Applies plugin denials and requires accepted Cloud or executor roots before merging skills. +pub(crate) fn apply_plugin_denials(catalog: &mut SkillCatalog, snapshot: &SelectedPluginSnapshot) { + for skill in &mut catalog.entries { + if !matches!( + skill.authority.kind, + SkillSourceKind::Cloud | SkillSourceKind::Executor + ) { + continue; + } + // Standalone skills do not require plugin acceptance. + if skill.remote_plugin_id.is_none() && skill.plugin_id.is_none() { + continue; + } + let accepted = if skill.authority.kind == SkillSourceKind::Executor { + let (Some(environment_id), Some(plugin_id)) = + (skill.main_prompt.environment_id(), &skill.plugin_id) + else { + skill.enabled = false; + continue; + }; + let root_id = format!("{environment_id}:{plugin_id}"); + snapshot + .plugins + .iter() + .any(|plugin| plugin.selected_root_id.as_deref() == Some(root_id.as_str())) + } else { + let Some(remote_plugin_id) = &skill.remote_plugin_id else { + skill.enabled = false; + continue; + }; + snapshot.plugins.iter().any(|plugin| { + plugin.selected_root_id.is_none() && plugin.plugin_id.as_str() == remote_plugin_id + }) + }; + skill.enabled &= skill + .remote_plugin_id + .as_ref() + .is_none_or(|id| !snapshot.disabled_plugin_ids.contains(id)) + && skill + .plugin_id + .as_ref() + .is_none_or(|id| !snapshot.disabled_plugin_ids.contains(id)) + && accepted; + } +} + +fn remote_key(entry: &SkillCatalogEntry) -> Option<(&str, &str)> { + Some((entry.remote_plugin_id.as_deref()?, entry.name.as_str())) +} + +/// Merges available Cloud counterparts, disabling unmatched remote skills only when Cloud is complete. +pub fn merge_catalogs(cloud: Option, executor: SkillCatalog) -> SkillCatalog { + let Some(mut merged) = cloud else { + return executor; + }; + let cloud_is_partial = merged.is_partial; + merged.is_partial |= executor.is_partial; + let cloud_entry_count = merged.entries.len(); + merged.warnings.extend(executor.warnings); + for mut entry in executor.entries { + let Some(key) = remote_key(&entry) else { + merged.entries.push(entry); + continue; + }; + // Executor entries cannot authorize or merge with one another. + let Some(cloud_entry) = merged.entries[..cloud_entry_count] + .iter_mut() + .find(|cloud_entry| remote_key(cloud_entry) == Some(key)) + else { + if !cloud_is_partial { + entry.enabled = false; + for source in &mut entry.sources { + source.enabled = false; + } + } + merged.entries.push(entry); + continue; + }; + let primary_source = entry.primary_source(); + for mut source in std::iter::once(primary_source).chain(entry.sources) { + source.enabled &= cloud_entry.enabled; + let already_present = cloud_entry.sources.iter().any(|existing| { + existing.authority == source.authority + && existing.package == source.package + && existing.main_prompt.environment_id() == source.main_prompt.environment_id() + }); + if !already_present { + cloud_entry.sources.push(source); + } + } + } + merged +} + /// Contents returned after resolving a skill resource through its owner. #[derive(Clone, Debug, PartialEq, Eq)] pub struct SkillReadResult { @@ -352,3 +500,7 @@ impl std::fmt::Display for SkillProviderError { impl std::error::Error for SkillProviderError {} pub type SkillProviderResult = Result; + +#[cfg(test)] +#[path = "catalog_tests.rs"] +mod tests; diff --git a/codex-rs/ext/skills/src/catalog_tests.rs b/codex-rs/ext/skills/src/catalog_tests.rs new file mode 100644 index 0000000000..eaba64b2f6 --- /dev/null +++ b/codex-rs/ext/skills/src/catalog_tests.rs @@ -0,0 +1,77 @@ +//! Tests partial Cloud reconciliation without changing source-specific enablement. + +use super::*; +use pretty_assertions::assert_eq; + +fn skill(kind: SkillSourceKind, name: &str) -> SkillCatalogEntry { + SkillCatalogEntry::new( + SkillPackageId(format!("skill://{kind}/{name}")), + SkillAuthority::new(kind.clone(), kind.to_string()), + name, + "", + SkillResourceId::new(format!("skill://{kind}/{name}/SKILL.md")), + ) + .with_remote_plugin_id(Some("plugin_a".to_string())) +} + +#[test] +fn partial_cloud_merges_matches_without_disabling_unmatched_installations() { + for (cloud_is_partial, cloud_has_match) in [ + (None, false), + (Some(false), false), + (Some(true), false), + (Some(false), true), + (Some(true), true), + ] { + for executor_is_partial in [false, true] { + let cloud_entry = skill(SkillSourceKind::Cloud, "matched"); + let matched = skill(SkillSourceKind::Executor, "matched").disabled(); + let mut unmatched = skill(SkillSourceKind::Executor, "unmatched"); + let mut denied_source = unmatched.primary_source(); + denied_source.enabled = false; + unmatched.sources.push(denied_source); + let denied = skill(SkillSourceKind::Executor, "denied").disabled(); + let mut executor = SkillCatalog::default(); + executor.extend(SkillCatalog { + entries: vec![matched.clone(), unmatched.clone(), denied.clone()], + is_partial: executor_is_partial, + ..Default::default() + }); + // Appending a complete provider must not clear an earlier partial result. + executor.extend(SkillCatalog::default()); + let cloud = cloud_is_partial.map(|is_partial| SkillCatalog { + entries: if cloud_has_match { + vec![cloud_entry.clone()] + } else { + Vec::new() + }, + is_partial, + // Diagnostic warnings alone do not make membership incomplete. + warnings: vec!["diagnostic".to_string()], + }); + let expected_match = if cloud_has_match { + let mut entry = cloud_entry; + entry.sources.push(matched.primary_source()); + entry + } else { + matched + }; + if cloud_is_partial == Some(false) { + unmatched.enabled = false; + } + assert_eq!( + merge_catalogs(cloud, executor), + SkillCatalog { + entries: vec![expected_match, unmatched, denied], + warnings: if cloud_is_partial.is_some() { + vec!["diagnostic".to_string()] + } else { + Vec::new() + }, + is_partial: cloud_is_partial.unwrap_or(false) || executor_is_partial, + }, + "cloud_is_partial={cloud_is_partial:?}, cloud_has_match={cloud_has_match}, executor_is_partial={executor_is_partial}", + ); + } + } +} diff --git a/codex-rs/ext/skills/src/cloud_cache_tests.rs b/codex-rs/ext/skills/src/cloud_cache_tests.rs index de05327814..5c20a304f9 100644 --- a/codex-rs/ext/skills/src/cloud_cache_tests.rs +++ b/codex-rs/ext/skills/src/cloud_cache_tests.rs @@ -1,6 +1,7 @@ //! Regression tests for cloud cache generation isolation and late completion. use super::*; +use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillResourceId; use crate::catalog::SkillSearchResult; use crate::provider::SkillProvider; @@ -17,6 +18,8 @@ use tokio::sync::Notify; #[derive(Default)] struct Provider { reads: AtomicUsize, + partial_list: AtomicBool, + next_catalog: Mutex>, pause_read: AtomicBool, pause_list: AtomicBool, entered: Notify, @@ -30,8 +33,17 @@ impl SkillProvider for Provider { self.entered.notify_one(); self.resume.notified().await; } + if let Some(catalog) = self.next_catalog.lock().unwrap().take() { + return Ok(catalog); + } + let is_partial = self.partial_list.swap(false, Ordering::SeqCst); Ok(SkillCatalog { - warnings: vec!["account catalog".into()], + is_partial, + warnings: if is_partial { + Vec::new() + } else { + vec!["account catalog".into()] + }, ..Default::default() }) }) @@ -89,7 +101,7 @@ fn query(client: Option>) -> SkillListQuery { include_cloud_skills: true, mcp_resources: client, executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, } } @@ -161,3 +173,112 @@ async fn late_discovery_cannot_publish_into_a_new_generation() { .is_none() ); } + +#[tokio::test] +async fn partial_cloud_catalog_without_warnings_is_refreshed_next_turn() { + let state = state(); + let provider = Arc::new(Provider::default()); + provider.partial_list.store(true, Ordering::SeqCst); + let providers = SkillProviders::new().with_cloud_provider(provider); + let client = Some(client()); + state + .refresh_cloud_catalog(&providers, query(client.clone())) + .await + .unwrap(); + assert_eq!( + state.available_cloud_catalog().unwrap(), + SkillCatalog { + is_partial: true, + ..Default::default() + } + ); + state + .refresh_cloud_catalog(&providers, query(client)) + .await + .unwrap(); + assert_eq!( + state.available_cloud_catalog().unwrap(), + SkillCatalog { + is_partial: false, + warnings: vec!["account catalog".into()], + ..Default::default() + } + ); +} + +#[tokio::test] +async fn cloud_refresh_preserves_complete_catalogs_only_within_the_same_auth_scope() { + let catalog = |name: &str, is_partial| SkillCatalog { + entries: vec![SkillCatalogEntry::new( + SkillPackageId("skill://demo".into()), + SkillAuthority::new(SkillSourceKind::Cloud, "codex_apps"), + name, + "test catalog", + SkillResourceId::new("skill://demo/SKILL.md"), + )], + // Complete listings can have diagnostics; these also keep discovery eligible for refresh. + warnings: vec![format!("{name} catalog diagnostic")], + is_partial, + }; + for previous_partial in [false, true] { + for incoming_partial in [false, true] { + for changed_auth in [false, true] { + let state = state(); + let provider = Arc::new(Provider::default()); + let providers = SkillProviders::new().with_cloud_provider(provider.clone()); + let original_client = client(); + let mut previous = catalog("previous", previous_partial); + *provider.next_catalog.lock().unwrap() = Some(previous.clone()); + state + .refresh_cloud_catalog(&providers, query(Some(original_client.clone()))) + .await + .unwrap(); + let generation = state.cloud_cache(Some(&original_client)); + let contents = state + .read_skill(&providers, request(Some(original_client.clone()))) + .await + .unwrap(); + + let incoming = catalog("incoming", incoming_partial); + *provider.next_catalog.lock().unwrap() = Some(incoming.clone()); + let current_client = if changed_auth { + client() + } else { + original_client + }; + state + .refresh_cloud_catalog(&providers, query(Some(current_client.clone()))) + .await + .unwrap(); + let retained = !changed_auth && !previous_partial && incoming_partial; + assert_eq!( + generation.catalog.as_ref().unwrap().is_partial, + previous_partial + ); + if retained { + previous.is_partial = true; + } + assert_eq!( + state.available_cloud_catalog(), + Some(if retained { previous } else { incoming }) + ); + assert!(!Arc::ptr_eq( + &generation, + &state.cloud_cache(Some(¤t_client)) + )); + assert_eq!( + state + .read_skill(&providers, request(Some(current_client))) + .await + .unwrap() + .contents, + if retained { + contents.contents + } else { + "contents 1".to_string() + }, + ); + } + } + } +} diff --git a/codex-rs/ext/skills/src/cloud_skill.rs b/codex-rs/ext/skills/src/cloud_skill.rs index 3f491e769f..5bdcfe74d1 100644 --- a/codex-rs/ext/skills/src/cloud_skill.rs +++ b/codex-rs/ext/skills/src/cloud_skill.rs @@ -16,10 +16,9 @@ use crate::state::SkillsThreadState; impl TurnLifecycleContributor for SkillsExtension { fn turn_start_phase(&self, thread_store: &ExtensionData) -> TurnStartPhase { - if self.providers.has_cloud_provider() - && thread_store - .get::() - .is_some_and(|state| state.cloud_skill_enabled()) + if thread_store + .get::() + .is_some_and(|state| state.cloud_skill_enabled()) { TurnStartPhase::RegularTaskStart } else { @@ -36,7 +35,7 @@ impl TurnLifecycleContributor for SkillsExtension { let Some(state) = input.thread_store.get::() else { return; }; - if !state.cloud_skill_enabled() || !self.providers.has_cloud_provider() { + if !state.cloud_skill_enabled() { return; } let config = state.config(); @@ -53,7 +52,7 @@ impl TurnLifecycleContributor for SkillsExtension { .get::() .and_then(|state| state.mcp_resources.clone()), executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }; if let Err(error) = state.refresh_cloud_catalog(&self.providers, query).await { self.emit_warning( diff --git a/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card_tests.rs b/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card_tests.rs index 892095ec27..3494f1a03a 100644 --- a/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card_tests.rs +++ b/codex-rs/ext/skills/src/dynamic_skill_selector/character_routing_card_tests.rs @@ -42,6 +42,7 @@ fn host_interface_metadata_makes_unlisted_workflow_discoverable() { }); let snapshot = HostSkillsSnapshot::new(std::sync::Arc::new(outcome)); let catalog = SkillCatalog { + is_partial: false, entries: vec![SkillCatalogEntry::new( SkillPackageId(resource.clone()), SkillAuthority::new(SkillSourceKind::Host, "host"), diff --git a/codex-rs/ext/skills/src/extension.rs b/codex-rs/ext/skills/src/extension.rs index 225e4c3c39..6cf682ea48 100644 --- a/codex-rs/ext/skills/src/extension.rs +++ b/codex-rs/ext/skills/src/extension.rs @@ -3,10 +3,8 @@ use std::sync::Arc; use crate::HostSkillsSnapshot; use crate::InjectedHostSkillPrompts; use codex_analytics::InvocationType; -use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; use codex_exec_server::LOCAL_ENVIRONMENT_ID; -use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_extension_api::ConfigContributor; use codex_extension_api::ContextContributor; use codex_extension_api::ContextualUserFragment; @@ -18,7 +16,6 @@ use codex_extension_api::ExtensionRegistryBuilder; use codex_extension_api::ExtensionWarning; use codex_extension_api::PromptFragment; use codex_extension_api::PromptSlot; -use codex_extension_api::SelectedPluginSnapshot; use codex_extension_api::SkillInvocationContributor; use codex_extension_api::SkillInvocationInput; use codex_extension_api::SkillInvocationKind; @@ -61,13 +58,13 @@ use crate::render_observability::CatalogSurface; use crate::render_observability::record_catalog_render; use crate::selection::collect_explicit_skill_mentions; use crate::shadow_selection_experiment::ShadowSelectionExperiment; +use crate::skills_extension_state::SkillToolAuthoritySelector; +use crate::skills_extension_state::SkillsStepState; use crate::sources::SkillProviders; -use crate::state::ExecutorSkillsStepState; use crate::state::HostSkillsCatalogInWorldState; use crate::state::HostSkillsStepState; use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; -use crate::state::SkillsTurnState; use crate::telemetry::SkillTelemetry; use crate::tools::SkillAnalytics; use crate::tools::SkillToolAuthority; @@ -148,10 +145,11 @@ where mcp_resources: input.mcp_resource_client.clone(), extension_metrics: input.extension_metrics.clone(), }); - let cloud_skills_available = !input - .environments - .iter() - .any(|environment| environment.environment_id == LOCAL_ENVIRONMENT_ID); + let cloud_skills_available = self.providers.has_cloud_provider() + && !input + .environments + .iter() + .any(|environment| environment.environment_id == LOCAL_ENVIRONMENT_ID); input.thread_store.insert(SkillsThreadState::new( (self.config_from_host)(input.config), cloud_skills_available, @@ -175,7 +173,7 @@ where if let Some(state) = thread_store.get::() { state.set_config(next_config); } else { - let cloud_skills_available = true; + let cloud_skills_available = self.providers.has_cloud_provider(); thread_store.insert(SkillsThreadState::new(next_config, cloud_skills_available)); } } @@ -199,23 +197,21 @@ where return Vec::new(); } let catalog = self - .list_skills( - SkillListQuery { - turn_id: thread_store.level_id().to_string(), - executor_roots: Vec::new(), - resolved_executor_roots: Vec::new(), - host_snapshot: None, - include_host_skills: false, - include_bundled_skills: config.bundled_skills_enabled, - include_cloud_skills: false, - mcp_resources: session_store - .get::() - .and_then(|state| state.mcp_resources.clone()), - executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), - }, - &thread_state, - ) + .providers + .list_for_turn(SkillListQuery { + turn_id: thread_store.level_id().to_string(), + executor_roots: Vec::new(), + resolved_executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: config.bundled_skills_enabled, + include_cloud_skills: false, + mcp_resources: session_store + .get::() + .and_then(|state| state.mcp_resources.clone()), + executor_capability_discovery: None, + executor_capability_discovery_v2: None, + }) .await; for warning in bounded_warnings(&catalog.warnings) { self.emit_warning(thread_store.level_id(), /*turn_id*/ None, warning); @@ -273,54 +269,17 @@ impl ToolContributor for SkillsExtension where C: Send + Sync + 'static, { - fn tools( - &self, - session_store: &ExtensionData, - thread_store: &ExtensionData, - ) -> Vec ToolExecutor>>> { - self.build_skill_tools( - session_store, - thread_store, - /*executor_query*/ None, - /*selected_plugins*/ None, - ) - } - fn tools_for_step( &self, session_store: &ExtensionData, thread_store: &ExtensionData, step_store: &ExtensionData, ) -> Vec ToolExecutor>>> { - let resolved_executor_roots = step_store - .get::>() - .map(|roots| roots.as_slice().to_vec()) - .unwrap_or_default(); - let executor_query = (!resolved_executor_roots.is_empty()).then(|| SkillListQuery { - turn_id: step_store.level_id().to_string(), - executor_roots: resolved_executor_roots - .iter() - .map(|root| root.selected_root().clone()) - .collect(), - resolved_executor_roots, - host_snapshot: None, - include_host_skills: false, - include_bundled_skills: false, - include_cloud_skills: false, - mcp_resources: None, - executor_capability_discovery: step_store - .get::() - .map(|discovery| discovery.as_ref().clone()), - executor_capability_discovery_v2: step_store - .get::>() - .map(|discovery| discovery.as_ref().clone()) - .unwrap_or_default(), - }); - self.build_skill_tools( + skill_tools( + self.providers.clone(), session_store, thread_store, - executor_query, - step_store.get::(), + step_store, ) } } @@ -386,16 +345,30 @@ where host_snapshot: host_snapshot.clone(), include_host_skills: host_skills.is_none() && !host_catalog_in_world_state, include_bundled_skills: config.bundled_skills_enabled, - include_cloud_skills: thread_state.cloud_skill_enabled(), + // Cloud and executor catalogs come from the prepared step snapshot. + include_cloud_skills: false, mcp_resources: mcp_resources.clone(), executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }; - let mut catalog = turn_store - .get::() - .map(|executor_skills| executor_skills.0.clone()) - .unwrap_or_default(); - catalog.extend(self.list_skills(query, &thread_state).await); + let step_state = input.step_store.get::(); + let step_catalog = step_state.as_ref().and_then(|state| state.get()); + let mut catalog = match &step_catalog { + Some(step_catalog) => step_catalog.select_from_catalog(&[ + SkillToolAuthoritySelector::Cloud, + SkillToolAuthoritySelector::Executor, + ]), + // Preserve legacy callers that select Cloud skills before World State preparation. + None if input + .step_store + .get::>() + .is_none() => + { + thread_state.available_cloud_catalog().unwrap_or_default() + } + None => SkillCatalog::default(), + }; + catalog.extend(self.providers.list_for_turn(query).await); for warning in bounded_warnings(&catalog.warnings) { self.emit_warning(thread_store.level_id(), Some(&input.turn_id), warning); } @@ -453,8 +426,6 @@ where } } - let mut warnings = catalog.warnings.clone(); - let mut main_prompts_injected = false; let mut injected_host_skill_prompts = InjectedHostSkillPrompts::default(); let analytics = SkillAnalytics::from_stores(session_store, thread_store); for entry in &selected_entries { @@ -481,7 +452,6 @@ where Some(&input.turn_id), warning.clone(), ); - warnings.push(warning); } let fragment = SkillInstructions { name: truncate_utf8_to_bytes(&entry.name, MAX_SKILL_NAME_BYTES).0, @@ -501,7 +471,6 @@ where }), }; fragments.push(Box::new(fragment)); - main_prompts_injected = true; if entry.authority.kind == SkillSourceKind::Host { injected_host_skill_prompts.insert_path(entry.main_prompt.as_str()); } else if let Some(analytics) = analytics.as_ref() @@ -509,6 +478,7 @@ where { analytics.track_skill_invocation( entry, + &entry.primary_source(), model_info.slug.clone(), input.turn_id.clone(), InvocationType::Explicit, @@ -522,7 +492,6 @@ where Some(&input.turn_id), warning.clone(), ); - warnings.push(warning); } } } @@ -545,12 +514,6 @@ where } } - turn_store.insert(SkillsTurnState { - catalog, - selected_entries, - warnings, - main_prompts_injected, - }); if !injected_host_skill_prompts.is_empty() { turn_store.insert(injected_host_skill_prompts); } @@ -561,39 +524,6 @@ where } impl SkillsExtension { - fn build_skill_tools( - &self, - session_store: &ExtensionData, - thread_store: &ExtensionData, - executor_query: Option, - selected_plugins: Option>, - ) -> Vec ToolExecutor>>> { - skill_tools( - self.providers.clone(), - session_store, - thread_store, - executor_query, - selected_plugins, - ) - } - - #[tracing::instrument(level = "trace", skip_all)] - async fn list_skills( - &self, - mut query: SkillListQuery, - thread_state: &SkillsThreadState, - ) -> SkillCatalog { - let include_cloud_skills = query.include_cloud_skills; - query.include_cloud_skills = false; - - let mut catalog = self.providers.list_for_turn(query).await; - if include_cloud_skills { - let cloud_catalog = thread_state.cloud_catalog_snapshot(); - catalog.extend(cloud_catalog); - } - catalog - } - #[tracing::instrument(level = "trace", skip_all, fields(skill = %entry.name))] async fn read_main_prompt( &self, diff --git a/codex-rs/ext/skills/src/invocation.rs b/codex-rs/ext/skills/src/invocation.rs index a20266d9dc..d27229e353 100644 --- a/codex-rs/ext/skills/src/invocation.rs +++ b/codex-rs/ext/skills/src/invocation.rs @@ -10,12 +10,37 @@ use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use crate::HostSkillsSnapshot; +use crate::catalog::SkillAuthority; +use crate::catalog::SkillCatalogEntry; +use crate::catalog::SkillResourceId; use crate::catalog::SkillSourceKind; -use crate::state::ExecutorSkillsStepState; +use crate::catalog::SkillSourceLocation; + +impl SkillCatalogEntry { + /// Uses merged identity and scope, with plugin attribution from the route actually used. + pub(crate) fn invocation( + &self, + source: &SkillSourceLocation, + invocation_type: InvocationType, + ) -> SkillInvocation { + SkillInvocation { + skill_name: self.name.clone(), + location: SkillInvocationLocation::Resource { + id: self.main_prompt.as_str().to_owned(), + skill_id: self.canonical_skill_id.clone(), + scope: self.analytics_scope, + }, + plugin_id: source.plugin_id.clone(), + remote_plugin_id: self.remote_plugin_id.clone(), + invocation_type, + } + } +} /// Identifies the executor-owned or host-owned skill referenced by a command. pub fn detect_implicit_skill_invocation( turn_store: &ExtensionData, + step_store: &ExtensionData, environment_id: &str, command: &str, workdir: &PathUri, @@ -42,39 +67,43 @@ pub fn detect_implicit_skill_invocation( }); } - let catalog = turn_store.get::()?; - for access in implicit_skill_accesses_for_command(command, workdir) { - for entry in &catalog.0.entries { - let Some((skill_environment_id, skill_path)) = entry.main_prompt.environment_path() - else { - continue; - }; - if !entry.enabled - || entry.authority.kind != SkillSourceKind::Executor - || skill_environment_id != environment_id + let accesses = implicit_skill_accesses_for_command(command, workdir); + if accesses.is_empty() { + return None; + } + let step_state = step_store.get::()?; + let step_catalog = step_state.get()?; + for access in accesses { + let matches_route = |authority: &SkillAuthority, main_prompt: &SkillResourceId| { + if authority.kind != SkillSourceKind::Executor + || main_prompt.environment_id() != Some(environment_id) { - continue; + return false; } - - let matches = match &access { + let Some((_, skill_path)) = main_prompt.environment_path() else { + return false; + }; + match &access { ImplicitSkillAccess::Document(path) => path == skill_path, ImplicitSkillAccess::Script(path) => skill_path .parent() .and_then(|skill_dir| skill_dir.join("scripts").ok()) .is_some_and(|scripts_dir| path.starts_with(&scripts_dir)), - }; - if matches { - return Some(SkillInvocation { - skill_name: entry.name.clone(), - location: SkillInvocationLocation::Resource { - id: entry.main_prompt.as_str().to_owned(), - skill_id: entry.canonical_skill_id.clone(), - scope: entry.analytics_scope, - }, - plugin_id: entry.plugin_id.clone(), - remote_plugin_id: None, - invocation_type: InvocationType::Implicit, - }); + } + }; + for entry in step_catalog.catalog.entries.iter().filter(|entry| { + matches!( + entry.authority.kind, + SkillSourceKind::Cloud | SkillSourceKind::Executor + ) + }) { + if entry.enabled && matches_route(&entry.authority, &entry.main_prompt) { + return Some(entry.invocation(&entry.primary_source(), InvocationType::Implicit)); + } + if let Some(source) = entry.sources.iter().find(|source| { + source.enabled && matches_route(&source.authority, &source.main_prompt) + }) { + return Some(entry.invocation(source, InvocationType::Implicit)); } } } diff --git a/codex-rs/ext/skills/src/invocation_tests.rs b/codex-rs/ext/skills/src/invocation_tests.rs index 30a88bc117..6dc3c7dca4 100644 --- a/codex-rs/ext/skills/src/invocation_tests.rs +++ b/codex-rs/ext/skills/src/invocation_tests.rs @@ -1,3 +1,6 @@ +use pretty_assertions::assert_eq; +use std::sync::Arc; + use super::*; use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; @@ -8,6 +11,7 @@ use crate::catalog::SkillResourceId; fn detect_executor_skill(command: &str, environments: &[&str]) -> Option { let skill_path = PathUri::parse("file:///skills/demo/SKILL.md").expect("valid skill URI"); let catalog = SkillCatalog { + is_partial: false, entries: environments .iter() .map(|environment| { @@ -27,10 +31,17 @@ fn detect_executor_skill(command: &str, environments: &[&str]) -> Option>, /// Present only when the opt-in high-level executor discovery path is selected. pub executor_capability_discovery: Option, - /// Inert V2 inventories; empty when none were supplied. Existing discovery is unchanged. - pub executor_capability_discovery_v2: Vec, + /// Present when V2 is selected, including empty or failed discovery. Never falls back to V1. + pub executor_capability_discovery_v2: Option>, } /// A skill read with the source access supplied by its callback. @@ -85,6 +85,7 @@ pub type SkillProviderFuture<'a, T> = /// provider must be read or searched through the same provider/authority rather /// than converted into an ambient local path. pub trait SkillProvider: Send + Sync { + /// Providers set `is_partial` when listing omits entries; warnings are diagnostic only. fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog>; fn read<'a>( diff --git a/codex-rs/ext/skills/src/provider/executor.rs b/codex-rs/ext/skills/src/provider/executor.rs index 632e09e6d4..56065f5338 100644 --- a/codex-rs/ext/skills/src/provider/executor.rs +++ b/codex-rs/ext/skills/src/provider/executor.rs @@ -3,6 +3,8 @@ use std::collections::HashSet; use std::sync::Arc; use codex_exec_server::EnvironmentAccess; +mod discovery_v2; + use codex_exec_server::EnvironmentManager; use codex_exec_server::FileSystemEnvironmentAccessor; use codex_extension_api::SelectedPluginSnapshot; @@ -87,6 +89,9 @@ pub(crate) fn attribute_executor_plugins( impl SkillProvider for ExecutorSkillProvider { fn list(&self, query: SkillListQuery) -> SkillProviderFuture<'_, SkillCatalog> { Box::pin(async move { + if let Some(snapshots) = &query.executor_capability_discovery_v2 { + return Ok(self.list_from_discovery_v2(snapshots)); + } if let Some(discovery) = query.executor_capability_discovery { return Ok(self.list_from_discovery(&discovery)); } diff --git a/codex-rs/ext/skills/src/provider/executor/discovery_v2.rs b/codex-rs/ext/skills/src/provider/executor/discovery_v2.rs new file mode 100644 index 0000000000..de01b5a3bc --- /dev/null +++ b/codex-rs/ext/skills/src/provider/executor/discovery_v2.rs @@ -0,0 +1,97 @@ +//! Projects executor-owned discovery metadata into skill catalogs without reopening host paths. + +use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; +use codex_exec_server::ExecutorSkill; +use codex_protocol::protocol::SkillScope; +use codex_skills::EnvironmentSkillMetadata; +use codex_utils_path_uri::PathUri; + +use crate::catalog::SkillAuthority; +use crate::catalog::SkillCatalog; +use crate::catalog::SkillCatalogEntry; +use crate::catalog::SkillSourceKind; + +use super::ExecutorSkillProvider; +use super::catalog_entry_from_skill; + +impl ExecutorSkillProvider { + pub(super) fn list_from_discovery_v2( + &self, + snapshots: &[ExecutorCapabilityDiscoveryV2Snapshot], + ) -> SkillCatalog { + let mut catalog = SkillCatalog::default(); + for snapshot in snapshots { + let discovered = match &snapshot.result { + Ok(discovered) => discovered, + Err(error) => { + catalog.is_partial = true; + catalog.warnings.push(format!( + "Environment `{}` discovery failed: {error}", + snapshot.environment_id + )); + continue; + } + }; + catalog.warnings.extend(discovered.warnings.clone()); + for plugin in &discovered.plugins { + for skill in &plugin.skills { + let Some(mut entry) = self.catalog_entry_from_discovered_skill( + skill, + &plugin.id, + &plugin.root, + &snapshot.environment_id, + ) else { + continue; + }; + entry.remote_plugin_id = plugin.remote_plugin_id.clone(); + entry.plugin_id = Some(plugin.id.clone()); + entry.analytics_scope = Some(SkillScope::User); + catalog.push_entry(entry); + } + } + for skill in &discovered.skills { + let Some(root) = skill.path.parent() else { + continue; + }; + if let Some(entry) = self.catalog_entry_from_discovered_skill( + skill, + "skills", + &root, + &snapshot.environment_id, + ) { + catalog.push_entry(entry); + } + } + } + catalog + } + + fn catalog_entry_from_discovered_skill( + &self, + skill: &ExecutorSkill, + root_id: &str, + root: &PathUri, + environment_id: &str, + ) -> Option { + let metadata = EnvironmentSkillMetadata { + path_to_skills_md: skill.path.clone(), + name: skill.name.clone(), + description: skill.description.clone(), + short_description: skill.short_description.clone(), + dependencies: skill.dependencies.clone(), + policy: skill.policy.clone(), + }; + if !metadata.matches_product_restriction(self.restriction_product) { + return None; + } + Some(catalog_entry_from_skill( + &metadata, + SkillAuthority::new(SkillSourceKind::Executor, root_id), + root_id, + root, + environment_id, + /*instructions*/ None, + &self.disabled_skill_paths, + )) + } +} diff --git a/codex-rs/ext/skills/src/provider/host.rs b/codex-rs/ext/skills/src/provider/host.rs index 78a80f324d..a5dd192b30 100644 --- a/codex-rs/ext/skills/src/provider/host.rs +++ b/codex-rs/ext/skills/src/provider/host.rs @@ -104,6 +104,7 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog { .map(|(index, root)| (root.as_path(), index)) .collect::>(); let mut catalog = SkillCatalog { + is_partial: false, entries: Vec::new(), warnings: outcome .errors diff --git a/codex-rs/ext/skills/src/render_tests.rs b/codex-rs/ext/skills/src/render_tests.rs index fa8072f7e7..5eb7873b7a 100644 --- a/codex-rs/ext/skills/src/render_tests.rs +++ b/codex-rs/ext/skills/src/render_tests.rs @@ -70,11 +70,13 @@ fn prebudget_dedup_can_choose_full_locators_over_preserved_aliases() { visible.authority.kind = SkillSourceKind::Executor; visible.id = SkillPackageId("skill://short/visible".to_string()); let executor = SkillCatalog { + is_partial: false, entries: vec![hidden.clone(), visible], warnings: Vec::new(), }; hidden.authority.kind = SkillSourceKind::Cloud; let cloud = SkillCatalog { + is_partial: false, entries: vec![hidden], warnings: Vec::new(), }; @@ -133,6 +135,7 @@ fn entry_with_path( #[test] fn ordering_follows_render_policy() { let catalog = SkillCatalog { + is_partial: false, entries: [ ("repo-zeta", SkillScope::Repo, "/skills/repo-zeta/SKILL.md"), ( @@ -217,6 +220,7 @@ fn ordering_follows_render_policy() { #[test] fn description_selection_follows_render_policy() { let catalog = SkillCatalog { + is_partial: false, entries: vec![ entry("shortened", "full description", Some("short description")), entry( @@ -297,6 +301,7 @@ fn catalog_budget_uses_context_percentage_or_character_fallback() { fn host_only_prompts_preserve_existing_behavior_with_and_without_aliases() { let root = "/Users/test/.codex/plugins/cache/openai-curated/host-plugin/1.0.0/skills-with-a-long-shared-root"; let unaliased_catalog = SkillCatalog { + is_partial: false, entries: [ ("alpha", "Alpha skill."), ("beta", "Beta skill."), @@ -311,6 +316,7 @@ fn host_only_prompts_preserve_existing_behavior_with_and_without_aliases() { warnings: Vec::new(), }; let catalog = SkillCatalog { + is_partial: false, entries: [ ("alpha", "Alpha skill."), ("beta", "Beta skill."), @@ -403,6 +409,7 @@ fn host_only_prompts_preserve_existing_behavior_with_and_without_aliases() { fn path_aliases_are_used_without_budget_pressure_when_they_reduce_prompt_size() { let root = "/Users/test/.codex/plugins/cache/openai-curated/example/hash/skills"; let catalog = SkillCatalog { + is_partial: false, entries: vec![ entry("alpha", "Alpha skill.", /*short_description*/ None) .with_display_path(format!("{root}/alpha/SKILL.md")) @@ -443,6 +450,7 @@ fn path_aliases_retain_every_skill_under_budget_pressure() { }) .collect::>(); let catalog = SkillCatalog { + is_partial: false, entries, warnings: Vec::new(), }; @@ -554,7 +562,7 @@ async fn host_alias_roots_follow_core_discovery_order() -> Result<(), Box>(); @@ -602,6 +610,7 @@ fn mixed_catalogs_keep_absolute_authority_aware_rendering_under_budget_pressure( .with_display_path("skill://executor/demo/SKILL.md"), ); let catalog = SkillCatalog { + is_partial: false, entries, warnings: Vec::new(), }; @@ -634,6 +643,7 @@ fn mixed_catalogs_keep_absolute_authority_aware_rendering_under_budget_pressure( #[test] fn mixed_catalog_reserves_executor_omission_marker_by_omitting_host_first() { let host_catalog = SkillCatalog { + is_partial: false, entries: vec![entry_with_path( "h", "", /*short_description*/ None, "/h", )], @@ -650,6 +660,7 @@ fn mixed_catalog_reserves_executor_omission_marker_by_omitting_host_first() { .with_display_path(resource) }; let executor_catalog = SkillCatalog { + is_partial: false, entries: vec![ executor_entry("e1", "skill://executor/one"), executor_entry( @@ -711,6 +722,7 @@ fn mixed_catalogs_alias_all_skill_sources_under_budget_pressure() { let host_root = "/Users/test/.codex/plugins/cache/openai-curated/example/hash1234567890/skills"; let catalog = |source: SkillSourceKind, authority: &str, root: &str, prefix: &str| SkillCatalog { + is_partial: false, entries: (0..3) .map(|index| { let name = format!("{prefix}-{index}"); @@ -797,6 +809,7 @@ fn mixed_catalogs_alias_all_skill_sources_under_budget_pressure() { fn mixed_catalog_prefers_executor_inclusion_over_total_aliased_inclusion() { let root = format!("/{}", "r".repeat(219)); let host_catalog = SkillCatalog { + is_partial: false, entries: ["h1", "h2"] .into_iter() .map(|name| { @@ -818,6 +831,7 @@ fn mixed_catalog_prefers_executor_inclusion_over_total_aliased_inclusion() { .with_display_path(resource) }; let executor_catalog = SkillCatalog { + is_partial: false, entries: vec![ executor_entry("e1", "skill://executor/one"), executor_entry("e2", &format!("skill://{}", "e".repeat(132))), @@ -826,6 +840,7 @@ fn mixed_catalog_prefers_executor_inclusion_over_total_aliased_inclusion() { }; let cloud_resource = "skill://cloud/one"; let cloud_catalog = SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId("c1".to_string()), @@ -923,7 +938,7 @@ async fn singleton_plugin_versions_share_the_marketplace_alias_root() include_cloud_skills: false, mcp_resources: None, executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }) .await?; let entries = catalog.entries.iter().collect::>(); @@ -957,6 +972,7 @@ async fn singleton_plugin_versions_share_the_marketplace_alias_root() #[test] fn omission_notice_follows_render_policy_and_is_charged_to_catalog_budget() { let catalog = SkillCatalog { + is_partial: false, entries: (0..20) .map(|index| { entry( @@ -1009,6 +1025,7 @@ fn omission_notice_follows_render_policy_and_is_charged_to_catalog_budget() { fn character_fallback_counts_multibyte_metadata_by_characters() { let description = "💡".repeat(MAX_CATALOG_SKILL_DESCRIPTION_CHARS); let catalog = SkillCatalog { + is_partial: false, entries: vec![ entry( "multibyte-one", @@ -1045,6 +1062,7 @@ fn character_fallback_counts_multibyte_metadata_by_characters() { #[test] fn catalog_report_counts_partial_description_truncation() { let catalog = SkillCatalog { + is_partial: false, entries: vec![entry( "partial", "abcdefghij", @@ -1090,6 +1108,7 @@ fn catalog_emits_omission_marker_when_every_minimum_skill_line_exceeds_budget() ) .with_display_path(format!("skill://{}", "x".repeat(512))); let catalog = SkillCatalog { + is_partial: false, entries: vec![oversized], warnings: Vec::new(), }; @@ -1151,6 +1170,7 @@ fn catalog_preserves_report_when_no_fragment_fits_budget() { ) .with_display_path(format!("skill://{}", "x".repeat(512))); let catalog = SkillCatalog { + is_partial: false, entries: vec![oversized], warnings: Vec::new(), }; diff --git a/codex-rs/ext/skills/src/required.rs b/codex-rs/ext/skills/src/required.rs index a13ed94cf5..1a99a69157 100644 --- a/codex-rs/ext/skills/src/required.rs +++ b/codex-rs/ext/skills/src/required.rs @@ -2,8 +2,7 @@ use codex_extension_api::ExtensionData; -use crate::state::ExecutorSkillsStepState; -use crate::state::SkillsThreadState; +use crate::skills_extension_state::SkillsStepState; /// Skill names that must be supplied by one selected environment before inference. pub struct EnvironmentSkillRequirements<'a> { @@ -11,47 +10,44 @@ pub struct EnvironmentSkillRequirements<'a> { pub skill_names: &'a [String], } -/// Validates requirements after discovery refreshes the turn's skill snapshots. +/// Validates requirements against the policy-checked catalog prepared for this step. pub fn validate_required_skills<'a>( - thread_store: &ExtensionData, - turn_store: &ExtensionData, + step_store: &ExtensionData, environments: impl IntoIterator>, ) -> Result<(), String> { let mut environments = environments.into_iter().peekable(); if environments.peek().is_none() { return Ok(()); } - let thread_state = thread_store.get::().ok_or_else(|| { + let step_state = step_store.get::().ok_or_else(|| { "Required skills cannot be validated because the skills extension is unavailable" .to_string() })?; - let executor = turn_store.get::(); - thread_state.with_cloud_catalog(|cloud| { - for EnvironmentSkillRequirements { - environment_id, - skill_names, - } in environments - { - for name in skill_names { - let available = executor - .iter() - .flat_map(|state| &state.0.entries) - .chain(cloud.into_iter().flat_map(|catalog| &catalog.entries)) - .any(|entry| { - entry.enabled - && entry.name == name.as_str() - && entry - .main_prompt - .environment_path() - .is_some_and(|(id, _)| id == environment_id) - }); - if !available { - return Err(format!( - "Required skill {name:?} from environment {environment_id:?} is unavailable" - )); - } + let step_catalog = step_state.get().ok_or_else(|| { + "Required skills cannot be validated because the step skill catalog is unavailable" + .to_string() + })?; + for EnvironmentSkillRequirements { + environment_id, + skill_names, + } in environments + { + for name in skill_names { + let available = step_catalog.catalog.entries.iter().any(|entry| { + entry.enabled + && entry.name == name.as_str() + && (entry.main_prompt.environment_id() == Some(environment_id) + || entry.sources.iter().any(|source| { + source.enabled + && source.main_prompt.environment_id() == Some(environment_id) + })) + }); + if !available { + return Err(format!( + "Required skill {name:?} from environment {environment_id:?} is unavailable" + )); } } - Ok(()) - }) + } + Ok(()) } diff --git a/codex-rs/ext/skills/src/selection.rs b/codex-rs/ext/skills/src/selection.rs index ba28ee866f..9d100f0c0d 100644 --- a/codex-rs/ext/skills/src/selection.rs +++ b/codex-rs/ext/skills/src/selection.rs @@ -105,12 +105,15 @@ fn push_selected( } fn entry_matches_path(entry: &SkillCatalogEntry, path: &str) -> bool { - entry.main_prompt.as_str() == path - || entry.id.0 == path - || entry - .display_path - .as_deref() - .is_some_and(|display_path| normalize_skill_path(display_path) == path) + entry.source_locations().any(|source| { + source.enabled + && (source.main_prompt.as_str() == path + || source.package.0 == path + || source + .display_path + .as_deref() + .is_some_and(|display_path| normalize_skill_path(display_path) == path)) + }) } fn path_is_skill(path: &str) -> bool { diff --git a/codex-rs/ext/skills/src/shadow_selection_experiment/experiment_tests.rs b/codex-rs/ext/skills/src/shadow_selection_experiment/experiment_tests.rs index b1154c32e7..1d79a82422 100644 --- a/codex-rs/ext/skills/src/shadow_selection_experiment/experiment_tests.rs +++ b/codex-rs/ext/skills/src/shadow_selection_experiment/experiment_tests.rs @@ -100,6 +100,7 @@ async fn turn_input_and_history_advance_while_ranking_is_blocked() for turn_id in ["first", "second"] { let turn_store = ExtensionData::new(turn_id); turn_store.insert(HostSkillsStepState(SkillCatalog { + is_partial: false, entries: ["x", "hidden", "selected"] .into_iter() .map(|name| { @@ -120,6 +121,7 @@ async fn turn_input_and_history_advance_while_ranking_is_blocked() Duration::from_secs(10), registry.turn_input_contributors()[0].contribute( TurnInputContext { + step_store: &turn_store, turn_id: turn_id.to_string(), user_input: vec![UserInput::Skill { name: "selected".to_string(), diff --git a/codex-rs/ext/skills/src/shadow_selection_experiment/mod.rs b/codex-rs/ext/skills/src/shadow_selection_experiment/mod.rs index 15c79a9efe..3270317679 100644 --- a/codex-rs/ext/skills/src/shadow_selection_experiment/mod.rs +++ b/codex-rs/ext/skills/src/shadow_selection_experiment/mod.rs @@ -95,7 +95,7 @@ impl ShadowSelectionExperiment { let mut eligible_ids = HashSet::new(); let mut eligible_skill_resources = HashSet::new(); for (id, entry) in catalog.entries.iter().enumerate() { - // Invocation observation exists only for host shell use and cloud reads. + // Invocation observation covers host shell use and reads of Cloud-primary skills. if !entry.is_model_visible() || !matches!( &entry.authority.kind, diff --git a/codex-rs/ext/skills/src/skills_extension_state.rs b/codex-rs/ext/skills/src/skills_extension_state.rs index f89246a218..ae2d350db9 100644 --- a/codex-rs/ext/skills/src/skills_extension_state.rs +++ b/codex-rs/ext/skills/src/skills_extension_state.rs @@ -5,6 +5,7 @@ use std::sync::Arc; use std::sync::Mutex; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; +use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_mcp::McpResourceClient; use codex_mcp::McpResourceClientAuthKey; use codex_mcp::McpResourceServerCacheKey; @@ -15,22 +16,33 @@ use crate::catalog::SkillCatalog; use crate::catalog::SkillPackageId; use crate::catalog::SkillReadResult; use crate::catalog::SkillResourceId; +use crate::catalog::SkillSourceKind; +use crate::catalog::merge_catalogs; use crate::provider::SkillReadRequest; +use schemars::JsonSchema; +use serde::Deserialize; +use serde::Serialize; +use tokio::sync::OnceCell; const MAX_CACHED_CLOUD_RESOURCES: usize = 100; const MAX_CACHED_CLOUD_CONTENT_BYTES: usize = 8 * 1024 * 1024; -/// Stores published cloud and executor catalogs for a thread. +/// Reusable provider catalogs, before step-specific plugin policy and merging. #[derive(Clone, Debug, Default)] pub(crate) struct SkillsExtensionState { - pub(crate) executor_cache: Vec, + // Roots-only discovery, reused within a turn. + pub(crate) executor_cache: Option, + // DiscoveryV1. pub(crate) executor_discovery_cache: Option, + // DiscoveryV2. + pub(crate) executor_v2_cache: Option, + // Shared by both discovery modes. pub(crate) cloud_cache: Option>, } /// Keeps cloud metadata and contents within one published auth scope and Apps availability state. /// Warning-free discovery is reused until the Apps resource generation changes. -/// A successful refresh replaces the Arc so late reads cannot populate its successor. +/// A published refresh replaces the Arc so late reads cannot populate its successor. pub(crate) struct CloudSkillGeneration { pub(crate) auth_cache_key: Option, pub(crate) resource_cache_key: Option, @@ -63,10 +75,57 @@ impl std::fmt::Debug for CloudSkillGeneration { } } -/// Retains the legacy per-root catalog until the thread is dropped. +/// Reuses the projection of unchanged successful V2 inventories, before plugin policy. +#[derive(Clone, Debug)] +pub(crate) struct CachedExecutorV2Catalog { + pub(crate) snapshots: Vec, + pub(crate) catalog: SkillCatalog, +} + +impl CachedExecutorV2Catalog { + pub(crate) fn matches( + &self, + snapshots: &[codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot], + ) -> bool { + self.snapshots.len() == snapshots.len() + && self + .snapshots + .iter() + .zip(snapshots) + .all(|(previous, current)| { + current.result.is_ok() && previous.same_discovery(current) + }) + } +} + +#[derive(Clone, Debug)] +pub(crate) struct ExecutorCatalogCacheKey { + pub(crate) turn_id: String, + pub(crate) resolved_roots: Vec, + pub(crate) include_bundled_skills: bool, +} + +impl ExecutorCatalogCacheKey { + pub(crate) fn matches(&self, query: &crate::provider::SkillListQuery) -> bool { + self.turn_id == query.turn_id + && self + .resolved_roots + .iter() + .map(ResolvedSelectedCapabilityRoot::selected_root) + .eq(query.executor_roots.iter()) + && self.include_bundled_skills == query.include_bundled_skills + // Unbound callers cannot establish identity for a cached filesystem catalog. + && query.resolved_executor_roots.len() == query.executor_roots.len() + && self.resolved_roots.iter().zip(&query.resolved_executor_roots).all(|(previous, current)| { + previous.selected_root() == current.selected_root() + && Arc::ptr_eq(previous.environment(), current.environment()) + }) + } +} + #[derive(Clone, Debug)] pub(crate) struct CachedExecutorCatalog { - pub(crate) root: SelectedCapabilityRoot, + pub(crate) key: ExecutorCatalogCacheKey, pub(crate) catalog: SkillCatalog, } @@ -166,3 +225,107 @@ impl std::fmt::Debug for CloudResourceCache { .finish_non_exhaustive() } } + +#[derive(Clone, Copy, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] +#[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] +pub(crate) enum SkillToolAuthoritySelector { + Cloud, + Executor, +} + +impl SkillToolAuthoritySelector { + pub(crate) fn matches(self, authority: &SkillAuthority) -> bool { + match self { + Self::Cloud => authority.kind == SkillSourceKind::Cloud, + Self::Executor => authority.kind == SkillSourceKind::Executor, + } + } +} + +/// Filled once by World State preparation; retained by this step's list/read tools. +pub(crate) type SkillsStepState = OnceCell>; + +/// Policy-checked, enabled skill routes for one step. Hidden skills remain explicitly readable. +#[derive(Debug)] +pub(crate) struct SkillStepCatalog { + pub(super) catalog: SkillCatalog, + /// None means no Cloud catalog was captured for this step. + pub(super) cloud_warnings: Option>, + pub(super) executor_warnings: Vec, +} + +impl SkillStepCatalog { + pub(crate) fn new( + cloud: Option, + executor: SkillCatalog, + discovery_v2: bool, + ) -> Self { + let cloud_skill_count = cloud.as_ref().map(|catalog| catalog.entries.len()); + let executor_skill_count = executor.entries.len(); + let cloud_warnings = cloud.as_ref().map(|catalog| catalog.warnings.clone()); + let executor_warnings = executor.warnings.clone(); + let mut catalog = if discovery_v2 { + merge_catalogs(cloud, executor) + } else { + let mut catalog = cloud.unwrap_or_default(); + catalog.is_partial |= executor.is_partial; + catalog.entries.extend(executor.entries); + catalog.warnings.extend(executor.warnings); + catalog + }; + catalog.entries.retain_mut(|entry| { + if !entry.enabled { + return false; + } + entry.sources.retain(|source| source.enabled); + true + }); + tracing::debug!( + discovery_v2, + ?cloud_skill_count, + executor_skill_count, + enabled_skill_count = catalog.entries.len(), + warning_count = catalog.warnings.len(), + "prepared step skill catalog" + ); + Self { + catalog, + cloud_warnings, + executor_warnings, + } + } + + /// Selects authorities from the prepared catalog without consulting thread state. + pub(crate) fn select_from_catalog( + &self, + authorities: &[SkillToolAuthoritySelector], + ) -> SkillCatalog { + let entries = self + .catalog + .entries + .iter() + .filter(|entry| { + authorities + .iter() + .any(|authority| authority.matches(&entry.authority)) + }) + .cloned() + .collect(); + let mut catalog = SkillCatalog { + is_partial: self.catalog.is_partial, + entries, + warnings: Vec::new(), + }; + if authorities.contains(&SkillToolAuthoritySelector::Cloud) + && let Some(warnings) = &self.cloud_warnings + { + catalog.warnings.extend(warnings.iter().cloned()); + } + if authorities.contains(&SkillToolAuthoritySelector::Executor) { + catalog + .warnings + .extend(self.executor_warnings.iter().cloned()); + } + catalog + } +} diff --git a/codex-rs/ext/skills/src/sources.rs b/codex-rs/ext/skills/src/sources.rs index 2d5d198250..445e552366 100644 --- a/codex-rs/ext/skills/src/sources.rs +++ b/codex-rs/ext/skills/src/sources.rs @@ -47,7 +47,9 @@ impl SkillProviderSource { fn should_list(&self, query: &SkillListQuery) -> bool { match &self.kind { SkillSourceKind::Host => query.include_host_skills, - SkillSourceKind::Executor => !query.executor_roots.is_empty(), + SkillSourceKind::Executor => { + query.executor_capability_discovery_v2.is_some() || !query.executor_roots.is_empty() + } SkillSourceKind::Cloud => query.include_cloud_skills, SkillSourceKind::Custom(_) => true, } diff --git a/codex-rs/ext/skills/src/state.rs b/codex-rs/ext/skills/src/state.rs index a7c4592cad..b1b94069f9 100644 --- a/codex-rs/ext/skills/src/state.rs +++ b/codex-rs/ext/skills/src/state.rs @@ -5,13 +5,11 @@ use std::sync::Mutex; use codex_exec_server::EnvironmentAccessKey; use codex_extension_api::ExtensionMetrics; use codex_mcp::McpResourceClient; -use codex_protocol::capabilities::SelectedCapabilityRoot; use crate::SkillsExtensionConfig; use crate::SkillsExtensionState; use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; -use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillPackageId; use crate::catalog::SkillProviderError; use crate::catalog::SkillProviderResult; @@ -25,8 +23,10 @@ use crate::shadow_selection_experiment::ShadowSelectionTurnState; use crate::shadow_selection_experiment::ShadowTaskContext; use crate::skills_extension_state::CachedExecutorCatalog; use crate::skills_extension_state::CachedExecutorDiscoveryCatalog; +use crate::skills_extension_state::CachedExecutorV2Catalog; use crate::skills_extension_state::CloudResourceCache; use crate::skills_extension_state::CloudSkillGeneration; +use crate::skills_extension_state::ExecutorCatalogCacheKey; use crate::skills_extension_state::SkillReadCacheKey; use crate::sources::SkillProviders; @@ -116,7 +116,7 @@ impl SkillsThreadState { .map(|turn| Arc::clone(&turn.state)) } - /// Returns the catalog for these exact roots, using the existing thread caches. + /// Returns the executor catalog, reusing matching discovery caches. #[tracing::instrument( name = "skills.executor.refresh_executor_catalog", level = "info", @@ -128,21 +128,63 @@ impl SkillsThreadState { providers: &SkillProviders, mut query: SkillListQuery, ) -> SkillCatalog { - if query.executor_capability_discovery.is_some() { - // High-level discovery is enabled: reuse or project its discovery snapshot. + query.include_bundled_skills = self.config().bundled_skills_enabled; + if let Some(snapshots) = &query.executor_capability_discovery_v2 { + let snapshots = snapshots.clone(); + let cached = self + .skills_extension_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .executor_v2_cache + .as_ref() + .filter(|cached| cached.matches(&snapshots)) + .map(|cached| cached.catalog.clone()); + match cached { + Some(catalog) => catalog, + None => { + let catalog = providers.list_executor_for_turn(query).await; + self.skills_extension_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .executor_v2_cache = Some(CachedExecutorV2Catalog { + snapshots, + catalog: catalog.clone(), + }); + catalog + } + } + } else if query.executor_capability_discovery.is_some() { self.executor_discovery_catalog_snapshot(providers, query) .await } else { - // High-level discovery is not enabled: retain the legacy per-root cache path. - let roots = std::mem::take(&mut query.executor_roots); - let mut catalog = SkillCatalog::default(); - for root in roots { - query.executor_roots = vec![root.clone()]; - catalog.extend( - self.executor_root_catalog(providers, root, query.clone()) - .await, - ); + if let Some(cached) = self + .skills_extension_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .executor_cache + .as_ref() + .filter(|cached| cached.key.matches(&query)) + { + return cached.catalog.clone(); } + + let key = ExecutorCatalogCacheKey { + turn_id: query.turn_id.clone(), + resolved_roots: query.resolved_executor_roots.clone(), + include_bundled_skills: query.include_bundled_skills, + }; + let catalog = if query.executor_roots.is_empty() { + SkillCatalog::default() + } else { + providers.list_executor_for_turn(query).await + }; + self.skills_extension_state + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .executor_cache = Some(CachedExecutorCatalog { + key, + catalog: catalog.clone(), + }); catalog } } @@ -198,66 +240,22 @@ impl SkillsThreadState { discovered } - #[tracing::instrument(name = "skills.executor.catalog_root", level = "info", skip_all)] - async fn executor_root_catalog( - &self, - providers: &SkillProviders, - root: SelectedCapabilityRoot, - query: SkillListQuery, - ) -> SkillCatalog { - if let Some(cached) = self - .skills_extension_state - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) - .executor_cache - .iter() - .find(|cached| cached.root == root) - { - return cached.catalog.clone(); - } - - let discovered = providers.list_executor_for_turn(query).await; - let mut state = self - .skills_extension_state - .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - let cache = &mut state.executor_cache; - if let Some(cached) = cache.iter().find(|cached| cached.root == root) { - return cached.catalog.clone(); - } - cache.push(CachedExecutorCatalog { - root, - catalog: discovered.clone(), - }); - discovered - } - - /// Reads the last authorized turn-start catalog without discovery or retries. - /// Same-auth failures may retain it; disabled or invalidated catalogs are empty. - pub fn cloud_catalog_snapshot(&self) -> SkillCatalog { - self.with_cloud_catalog(|catalog| catalog.cloned().unwrap_or_default()) - } - - /// Borrows the authorized catalog under the cache lock. The reader must not re-enter this state. - pub(crate) fn with_cloud_catalog(&self, read: impl FnOnce(Option<&SkillCatalog>) -> T) -> T { + /// Returns the cached Cloud catalog when enabled and authorized, without refreshing it. + pub(crate) fn available_cloud_catalog(&self) -> Option { let config = self .config .lock() .unwrap_or_else(std::sync::PoisonError::into_inner); if !config.cloud_skill_enabled || !self.cloud_skills_available { - return read(None); + return None; } - let state = self - .skills_extension_state + self.skills_extension_state .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner); - read( - state - .cloud_cache - .as_ref() - .filter(|cache| cache.is_current()) - .and_then(|cache| cache.catalog.as_ref()), - ) + .unwrap_or_else(std::sync::PoisonError::into_inner) + .cloud_cache + .as_ref() + .filter(|cache| cache.is_current()) + .and_then(|cache| cache.catalog.clone()) } fn cloud_cache(&self, mcp_resources: Option<&McpResourceClient>) -> Arc { @@ -305,7 +303,7 @@ impl SkillsThreadState { && cache .catalog .as_ref() - .is_some_and(|catalog| catalog.warnings.is_empty()) + .is_some_and(|catalog| !catalog.is_partial && catalog.warnings.is_empty()) && cache.resource_cache_key == resource_cache_key { return Ok(()); @@ -327,12 +325,28 @@ impl SkillsThreadState { "cloud skill cache changed during discovery", )); } + let resources = if catalog.is_partial + && let Some(previous) = cache + .catalog + .as_ref() + .filter(|previous| !previous.is_partial) + { + catalog = previous.clone(); + catalog.is_partial = true; + cache + .resources + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone() + } else { + CloudResourceCache::default() + }; catalogs.cloud_cache = Some(Arc::new(CloudSkillGeneration { auth_cache_key: cache.auth_cache_key.clone(), resource_cache_key, mcp_resources: cache.mcp_resources.clone(), catalog: Some(catalog), - resources: Mutex::new(CloudResourceCache::default()), + resources: Mutex::new(resources), })); Ok(()) } @@ -438,19 +452,8 @@ impl EmittedCatalogBudgetWarnings { } } -#[derive(Clone, Debug, Default, PartialEq, Eq)] -pub(crate) struct SkillsTurnState { - pub(crate) catalog: SkillCatalog, - pub(crate) selected_entries: Vec, - pub(crate) warnings: Vec, - pub(crate) main_prompts_injected: bool, -} - #[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] pub(crate) struct HostSkillsCatalogInWorldState; -#[derive(Clone, Debug, Default)] -pub(crate) struct ExecutorSkillsStepState(pub(crate) SkillCatalog); - #[derive(Clone, Debug, Default)] pub(crate) struct HostSkillsStepState(pub(crate) SkillCatalog); diff --git a/codex-rs/ext/skills/src/tools/list.rs b/codex-rs/ext/skills/src/tools/list.rs index 7d7c64c6d5..ff981decd3 100644 --- a/codex-rs/ext/skills/src/tools/list.rs +++ b/codex-rs/ext/skills/src/tools/list.rs @@ -86,7 +86,7 @@ impl<'call> ToolExecutor> for ListTool { Box::pin(async move { let args: ListArgs = parse_args(&call)?; let response_byte_budget = call.response_byte_budget(MAX_SKILL_RESPONSE_BYTES); - let catalog = self.context.catalog(&call.turn_id, args.authority).await; + let catalog = self.context.catalog(&[args.authority])?; let mut omitted_oversized_entry = false; let canonical_skills = catalog .entries diff --git a/codex-rs/ext/skills/src/tools/mod.rs b/codex-rs/ext/skills/src/tools/mod.rs index 64d2eaddde..094d62ac6a 100644 --- a/codex-rs/ext/skills/src/tools/mod.rs +++ b/codex-rs/ext/skills/src/tools/mod.rs @@ -5,15 +5,14 @@ use std::sync::Arc; use codex_analytics::AnalyticsEventsClient; use codex_analytics::InvocationType; -use codex_analytics::SkillInvocation; -use codex_analytics::SkillInvocationLocation; use codex_analytics::build_track_events_context; +use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; +use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_extension_api::ExtensionData; use codex_extension_api::ExtensionMetrics; use codex_extension_api::FunctionCallError; use codex_extension_api::JsonToolOutput; use codex_extension_api::ResponsesApiTool; -use codex_extension_api::SelectedPluginSnapshot; use codex_extension_api::ThreadOriginator; use codex_extension_api::ToolCall; use codex_extension_api::ToolExecutor; @@ -34,14 +33,14 @@ use schemars::JsonSchema; use serde::Deserialize; use serde::Serialize; use serde_json::Value; -use tokio::sync::OnceCell; use crate::catalog::SkillAuthority; use crate::catalog::SkillCatalog; use crate::catalog::SkillCatalogEntry; use crate::catalog::SkillSourceKind; -use crate::provider::SkillListQuery; -use crate::provider::attribute_executor_plugins; +use crate::catalog::SkillSourceLocation; +use crate::skills_extension_state::SkillToolAuthoritySelector; +use crate::skills_extension_state::SkillsStepState; use crate::sources::SkillProviders; use crate::state::SkillsSessionState; use crate::state::SkillsThreadState; @@ -60,14 +59,18 @@ pub(crate) fn skill_tools( providers: SkillProviders, session_store: &ExtensionData, thread_store: &ExtensionData, - executor_query: Option, - selected_plugins: Option>, + step_store: &ExtensionData, ) -> Vec ToolExecutor>>> { let Some(thread_state) = thread_store.get::() else { return Vec::new(); }; - let cloud_available = providers.has_cloud_provider() && thread_state.cloud_skill_enabled(); - if !cloud_available && executor_query.is_none() { + let has_executor = step_store + .get::>() + .is_some_and(|roots| !roots.is_empty()) + || step_store + .get::>() + .is_some(); + if !thread_state.cloud_skill_enabled() && !has_executor { return Vec::new(); } let mcp_resources = session_store @@ -79,10 +82,7 @@ pub(crate) fn skill_tools( mcp_resources, thread_state, analytics, - cloud_available, - executor_query, - selected_plugins, - executor_catalog: Arc::new(OnceCell::new()), + step_catalog: step_store.get_or_init(SkillsStepState::new), }; vec![ Arc::new(list::ListTool { @@ -131,10 +131,12 @@ impl SkillAnalytics { pub(crate) fn track_skill_invocation( &self, skill: &SkillCatalogEntry, + source: &SkillSourceLocation, model: String, turn_id: String, invocation_type: InvocationType, ) { + let invocation = skill.invocation(source, invocation_type); if let Some(telemetry) = &self.telemetry { telemetry .as_ref() @@ -144,7 +146,7 @@ impl SkillAnalytics { turn_id: &turn_id, skill_name: &skill.name, scope: skill.analytics_scope, - plugin_id: skill.plugin_id.as_deref(), + plugin_id: invocation.plugin_id.as_deref(), invocation_type: match invocation_type { InvocationType::Explicit => SkillInvocationType::Explicit, InvocationType::Implicit => SkillInvocationType::Implicit, @@ -156,12 +158,13 @@ impl SkillAnalytics { .as_ref() .filter(|turn| turn.turn_id == turn_id); if let Some(turn_metrics) = &turn_metrics { - turn_metrics.record_plugin(skill.plugin_id.as_deref()); + turn_metrics.record_plugin(invocation.plugin_id.as_deref()); } if let Some(metrics) = &self.metrics { let skill_name_tag = sanitize_metric_tag_value(skill.name.as_str()); - let plugin_id_tag = - sanitize_metric_tag_value(skill.plugin_id.as_deref().unwrap_or("unattributed")); + let plugin_id_tag = sanitize_metric_tag_value( + invocation.plugin_id.as_deref().unwrap_or("unattributed"), + ); let model_slug_tag = sanitize_metric_tag_value(model.as_str()); let reasoning_effort = turn_metrics .as_ref() @@ -192,17 +195,7 @@ impl SkillAnalytics { self.product_client_id.clone(), turn_metrics.and_then(|turn| turn.turn_metadata.clone()), ), - vec![SkillInvocation { - skill_name: skill.name.clone(), - location: SkillInvocationLocation::Resource { - id: skill.main_prompt.as_str().to_string(), - skill_id: skill.canonical_skill_id.clone(), - scope: skill.analytics_scope, - }, - plugin_id: skill.plugin_id.clone(), - remote_plugin_id: None, - invocation_type, - }], + vec![invocation], ); } } @@ -213,53 +206,20 @@ struct SkillToolContext { mcp_resources: Option>, thread_state: Arc, analytics: Option, - cloud_available: bool, - executor_query: Option, - selected_plugins: Option>, - executor_catalog: Arc>, + step_catalog: Arc, } impl SkillToolContext { - async fn catalog(&self, turn_id: &str, authority: SkillToolAuthoritySelector) -> SkillCatalog { - match authority { - SkillToolAuthoritySelector::Cloud => { - if !self.cloud_available { - return SkillCatalog::default(); - } - self.thread_state.cloud_catalog_snapshot() - } - SkillToolAuthoritySelector::Executor => { - let Some(mut query) = self.executor_query.clone() else { - return SkillCatalog::default(); - }; - query.turn_id = turn_id.to_string(); - let mut catalog = self - .executor_catalog - .get_or_init(|| self.providers.list_executor_for_turn(query)) - .await - .clone(); - if let Some(selected_plugins) = &self.selected_plugins { - attribute_executor_plugins(&mut catalog, selected_plugins); - } - catalog - } - } - } -} - -#[derive(Clone, Copy, Debug, Deserialize, Eq, JsonSchema, PartialEq, Serialize)] -#[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] -enum SkillToolAuthoritySelector { - Cloud, - Executor, -} - -impl SkillToolAuthoritySelector { - fn matches(self, authority: &SkillAuthority) -> bool { - match self { - Self::Cloud => authority.kind == SkillSourceKind::Cloud, - Self::Executor => authority.kind == SkillSourceKind::Executor, - } + fn catalog( + &self, + authorities: &[SkillToolAuthoritySelector], + ) -> Result { + let snapshot = self.step_catalog.get().ok_or_else(|| { + FunctionCallError::RespondToModel( + "skill catalog is not ready for this step".to_string(), + ) + })?; + Ok(snapshot.select_from_catalog(authorities)) } } diff --git a/codex-rs/ext/skills/src/tools/read.rs b/codex-rs/ext/skills/src/tools/read.rs index c502173b6f..b34353ed0a 100644 --- a/codex-rs/ext/skills/src/tools/read.rs +++ b/codex-rs/ext/skills/src/tools/read.rs @@ -87,6 +87,10 @@ impl<'call> ToolExecutor> for ReadTool { validate_handle("resource", resource, MAX_HANDLE_BYTES)?; } + let catalog = self.context.catalog(&[ + SkillToolAuthoritySelector::Cloud, + SkillToolAuthoritySelector::Executor, + ])?; let search_authorities: &[_] = match args.package.alias_source() { Some(SkillSourceKind::Cloud) => &[SkillToolAuthoritySelector::Cloud], Some(SkillSourceKind::Executor) => &[SkillToolAuthoritySelector::Executor], @@ -97,36 +101,47 @@ impl<'call> ToolExecutor> for ReadTool { }; let mut selected_skill = None; for &selector in search_authorities { - let catalog = self.context.catalog(&call.turn_id, selector).await; + // Resolve prompt aliases within their primary-authority partition. + let entries = catalog + .entries + .iter() + .filter(|entry| selector.matches(&entry.authority)) + .collect::>(); let alias_plan = build_alias_plan( - &catalog - .entries + &entries .iter() + .copied() .filter(|entry| entry.is_model_visible()) .collect::>(), ); - if let Some(entry) = catalog.entries.into_iter().find(|entry| { - entry.enabled - && (entry.id.0 == args.package.as_str() - || alias_plan - .as_ref() - .and_then(|plan| plan.shorten(&entry.id.0)) - .is_some_and(|alias| alias == args.package.as_str())) - && SkillToolAuthority::from_authority(&entry.authority) - .is_some_and(|authority| authority.selector() == selector) - }) { - selected_skill = Some((entry, selector)); + selected_skill = entries.into_iter().find_map(|entry| { + let matches_alias = alias_plan + .as_ref() + .and_then(|plan| plan.shorten(&entry.id.0)) + .is_some_and(|alias| alias == args.package.as_str()); + entry.source_locations().find_map(|source| { + if !(source.package.0 == args.package.as_str() + || (source.package == entry.id && matches_alias)) + { + return None; + } + let output_authority = + SkillToolAuthority::from_authority(&source.authority)?.selector(); + Some((entry.clone(), source, output_authority)) + }) + }); + if selected_skill.is_some() { break; } } - let Some((skill_entry, output_authority)) = selected_skill else { + let Some((skill_entry, selected_source, output_authority)) = selected_skill else { return Err(FunctionCallError::RespondToModel( "skill package is not available".to_string(), )); }; - let authority = skill_entry.authority.clone(); - let package = skill_entry.id.clone(); - let main_prompt = skill_entry.main_prompt.clone(); + let authority = selected_source.authority.clone(); + let package = selected_source.package.clone(); + let main_prompt = selected_source.main_prompt.clone(); let requested_resource = match args.resource { None => main_prompt.clone(), Some(resource) if resource == main_prompt.as_str() => main_prompt.clone(), @@ -216,14 +231,6 @@ impl<'call> ToolExecutor> for ReadTool { "skill provider returned a different resource".to_string(), )); } - if output_authority == super::SkillToolAuthoritySelector::Cloud - && let Some(state) = self - .context - .thread_state - .shadow_selection_turn(&call.turn_id) - { - state.record_invocation(main_prompt.as_str()); - } let start = parse_pagination_cursor( args.cursor.as_deref(), result.contents.as_str(), @@ -237,6 +244,15 @@ impl<'call> ToolExecutor> for ReadTool { "skills.read cursor is invalid".to_string(), )); } + // Shadow candidates use the merged identity, not the selected read route. + if skill_entry.authority.kind == SkillSourceKind::Cloud + && let Some(state) = self + .context + .thread_state + .shadow_selection_turn(&call.turn_id) + { + state.record_invocation(skill_entry.main_prompt.as_str()); + } let skill_root = if output_authority == super::SkillToolAuthoritySelector::Executor { main_prompt .environment_path() @@ -278,6 +294,7 @@ impl<'call> ToolExecutor> for ReadTool { { analytics.track_skill_invocation( &skill_entry, + &selected_source, call.model.clone(), call.turn_id.clone(), InvocationType::Implicit, diff --git a/codex-rs/ext/skills/src/world_state_catalogs.rs b/codex-rs/ext/skills/src/world_state_catalogs.rs index 1998e07b3e..2c05462f70 100644 --- a/codex-rs/ext/skills/src/world_state_catalogs.rs +++ b/codex-rs/ext/skills/src/world_state_catalogs.rs @@ -1,5 +1,6 @@ use std::sync::Arc; +use codex_exec_server::ResolvedSelectedCapabilityRoot; use codex_extension_api::ContextualUserFragment; use codex_extension_api::ExtensionEventSink; use codex_extension_api::ExtensionWarning; @@ -12,6 +13,7 @@ use serde::Serialize; use crate::HostSkillsSnapshot; use crate::SkillsExtensionConfig; use crate::catalog::SkillCatalog; +use crate::catalog::apply_plugin_denials; use crate::provider::SkillListQuery; use crate::provider::attribute_executor_plugins; use crate::render::AvailableSkillsRender; @@ -24,9 +26,11 @@ use crate::render::render_prepared_skill_catalogs; use crate::render::skill_metadata_budget; use crate::render_observability::CatalogSurface; use crate::render_observability::record_catalog_render; +use crate::skills_extension_state::SkillStepCatalog; +use crate::skills_extension_state::SkillToolAuthoritySelector; +use crate::skills_extension_state::SkillsStepState; use crate::sources::SkillProviders; use crate::state::EmittedCatalogBudgetWarnings; -use crate::state::ExecutorSkillsStepState; use crate::state::HostSkillsCatalogInWorldState; use crate::state::HostSkillsStepState; use crate::state::SkillsSessionState; @@ -152,12 +156,28 @@ impl<'a> CatalogContext<'a> { } pub(crate) async fn discover_catalogs(&self) -> CatalogContributions { - let cloud_enabled = - self.thread_state.cloud_skill_enabled() && self.providers.has_cloud_provider(); + let cloud_enabled = self.thread_state.cloud_skill_enabled(); + // Core always supplies this vector, including when no captured roots are ready. + // Descriptor-only callers retain their unbound discovery path. + let captured_roots = self + .input + .step_store + .get::>(); + let executor_roots = captured_roots + .as_ref() + .map(|roots| { + roots + .iter() + .map(|root| root.selected_root().clone()) + .collect() + }) + .unwrap_or_else(|| self.input.ready_selected_capability_roots.to_vec()); let query = SkillListQuery { turn_id: self.input.turn_id.to_string(), - executor_roots: self.input.ready_selected_capability_roots.to_vec(), - resolved_executor_roots: Vec::new(), + executor_roots, + resolved_executor_roots: captured_roots + .map(|roots| roots.as_ref().clone()) + .unwrap_or_default(), host_snapshot: None, include_host_skills: false, include_bundled_skills: self.config.bundled_skills_enabled, @@ -171,59 +191,53 @@ impl<'a> CatalogContext<'a> { executor_capability_discovery_v2: self .input .executor_capability_discovery_v2 - .map(Vec::from) - .unwrap_or_default(), + .map(Vec::from), }; - let cloud = self.cloud_catalog_contribution(&query); - let (executor, host) = futures::join!( - self.discover_executor_catalog(query), + let selected_plugins = self.input.step_store.get::(); + let step_state = self.input.step_store.get_or_init(SkillsStepState::new); + let (step_catalog, host) = futures::join!( + step_state.get_or_init(|| async { + let discovery_v2_enabled = query.executor_capability_discovery_v2.is_some(); + let mut executor = self + .thread_state + .refresh_executor_catalog(self.providers, query) + .await; + let mut cloud = self.thread_state.available_cloud_catalog(); + if let Some(selected_plugins) = selected_plugins.as_deref() { + if discovery_v2_enabled { + apply_plugin_denials(&mut executor, selected_plugins); + if let Some(cloud) = &mut cloud { + apply_plugin_denials(cloud, selected_plugins); + } + } else { + attribute_executor_plugins(&mut executor, selected_plugins); + } + } + Arc::new(SkillStepCatalog::new(cloud, executor, discovery_v2_enabled)) + }), self.discover_host_catalog(), ); - + let cloud_status = if !self.providers.has_cloud_provider() { + CatalogStatus::Unavailable + } else if cloud_enabled || step_catalog.cloud_warnings.is_some() { + CatalogStatus::Enabled + } else { + CatalogStatus::Disabled + }; CatalogContributions { - executor, - cloud, + executor: CatalogContribution { + catalog: step_catalog.select_from_catalog(&[SkillToolAuthoritySelector::Executor]), + status: CatalogStatus::Enabled, + }, + cloud: CatalogContribution { + catalog: step_catalog.select_from_catalog(&[SkillToolAuthoritySelector::Cloud]), + status: cloud_status, + }, host, } } - async fn discover_executor_catalog(&self, query: SkillListQuery) -> CatalogContribution { - let mut catalog = self - .thread_state - .refresh_executor_catalog(self.providers, query) - .await; - if let Some(selected_plugins) = self.input.step_store.get::() { - attribute_executor_plugins(&mut catalog, &selected_plugins); - } - self.input - .turn_store - .insert(ExecutorSkillsStepState(catalog.clone())); - - CatalogContribution { - catalog, - status: CatalogStatus::Enabled, - } - } - - fn cloud_catalog_contribution(&self, query: &SkillListQuery) -> CatalogContribution { - if !self.providers.has_cloud_provider() { - return CatalogContribution::unavailable(); - } - - if !query.include_cloud_skills { - return CatalogContribution { - catalog: SkillCatalog::default(), - status: CatalogStatus::Disabled, - }; - } - - CatalogContribution { - catalog: self.thread_state.cloud_catalog_snapshot(), - status: CatalogStatus::Enabled, - } - } - async fn discover_host_catalog(&self) -> CatalogContribution { let Some(host_snapshot) = self .input @@ -249,7 +263,7 @@ impl<'a> CatalogContext<'a> { include_cloud_skills: false, mcp_resources: None, executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }) .await; self.input diff --git a/codex-rs/ext/skills/tests/executor_file_system_authority.rs b/codex-rs/ext/skills/tests/executor_file_system_authority.rs index c2ab52f9fe..24c067b023 100644 --- a/codex-rs/ext/skills/tests/executor_file_system_authority.rs +++ b/codex-rs/ext/skills/tests/executor_file_system_authority.rs @@ -53,6 +53,7 @@ use codex_skills_extension::catalog::SkillAuthority; use codex_skills_extension::catalog::SkillCatalog; use codex_skills_extension::catalog::SkillCatalogEntry; use codex_skills_extension::catalog::SkillPackageId; +use codex_skills_extension::catalog::SkillProviderError; #[cfg(windows)] use codex_skills_extension::catalog::SkillReadResult; use codex_skills_extension::catalog::SkillResourceId; @@ -312,6 +313,77 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() { ); } +#[tokio::test] +async fn reads_use_callback_access_without_capability_roots() { + let test_root = create_local_skill_root("captured").expect("create skill"); + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let environment = manager.get_environment("local").expect("local environment"); + let fs = environment.get_filesystem(); + let access = FileSystemEnvironmentAccessor::unrestricted(&fs); + let provider = ExecutorSkillProvider::new_with_restriction_product( + manager, /*restriction_product*/ None, + ); + // This ID is absent from the provider's registry; the callback owns its filesystem access. + let resource = SkillResourceId::environment( + "skill://root/skill/SKILL.md", + "captured", + PathUri::from_host_native_path(test_root.join("skill/SKILL.md")).unwrap(), + ); + let result = provider + .read(SkillReadRequest { + authority: SkillAuthority::new(SkillSourceKind::Executor, "root"), + package: SkillPackageId("skill://root/skill".to_string()), + resource: resource.clone(), + context: SkillReadContext::Executor { fs: &access }, + }) + .await; + assert_eq!( + result, + Ok(codex_skills_extension::catalog::SkillReadResult { + resource, + contents: SKILL_CONTENTS.to_string(), + }) + ); + std::fs::remove_dir_all(test_root).expect("remove skill directory"); +} + +#[tokio::test] +async fn executor_skill_reads_reject_invalid_resource_handles() { + let access = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); + let provider = ExecutorSkillProvider::new_with_restriction_product( + Arc::new(EnvironmentManager::default_for_tests()), + /*restriction_product*/ None, + ); + for (resource, message) in [ + ( + "skill://root/package/../secret", + "executor skill resource does not match its package", + ), + ( + "skill://other/package/SKILL.md", + "executor skill resource does not match its package", + ), + ( + "skill://root/other/SKILL.md", + "executor skill resource does not match its package", + ), + ( + "skill://root/package/SKILL.md", + "executor skill resource is not bound to an environment", + ), + ] { + let result = provider + .read(SkillReadRequest { + authority: SkillAuthority::new(SkillSourceKind::Executor, "root"), + package: SkillPackageId("skill://root/package".to_string()), + resource: SkillResourceId::new(resource), + context: SkillReadContext::Executor { fs: &access }, + }) + .await; + assert_eq!(result, Err(SkillProviderError::new(message)), "{resource}"); + } +} + /// Restricted skill reads must fail closed when the Windows executor has no sandbox selected. #[cfg(windows)] #[tokio::test] @@ -417,7 +489,7 @@ async fn selected_root_id_distinguishes_identical_executor_paths() { include_cloud_skills: false, mcp_resources: None, executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }) .await .expect("list executor skills"); @@ -508,7 +580,7 @@ async fn executor_discovery_preserves_posix_and_windows_locator_alias_roots() { include_cloud_skills: false, mcp_resources: None, executor_capability_discovery: Some(discovery), - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }) .await .expect("list executor skills"); @@ -517,6 +589,7 @@ async fn executor_discovery_preserves_posix_and_windows_locator_alias_roots() { assert_eq!( catalog, SkillCatalog { + is_partial: false, entries: vec![ SkillCatalogEntry::new( SkillPackageId(format!("{alias_root}/skill")), @@ -626,20 +699,21 @@ async fn executor_discovery_routes_produce_equivalent_catalog_metadata() { include_cloud_skills: false, mcp_resources: None, executor_capability_discovery, - executor_capability_discovery_v2: vec![ - codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot { - environment_id: "local".to_string(), - result: Err("V2 must not change V1 or direct discovery".to_string()), - }, - ], + executor_capability_discovery_v2: None, }; let direct = provider .list(query(None)) .await .expect("list directly discovered executor skills"); - let discovery = ExecutorCapabilityDiscoveryCache::new(manager) - .snapshot(&executor_roots, &Default::default()) + let resolved_roots = manager + .resolve_selected_capability_roots( + &executor_roots, + &HashMap::from([("local".to_string(), manager.get_environment("local"))]), + ) + .await; + let discovery = ExecutorCapabilityDiscoveryCache::default() + .snapshot(&resolved_roots, &Default::default()) .await; let bundled = provider .list(query(Some(discovery.clone()))) @@ -770,8 +844,14 @@ async fn pre_discovered_executor_catalog_snapshot() { path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), }, }]; - let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::new(manager) - .snapshot(&executor_roots, &Default::default()) + let resolved_roots = manager + .resolve_selected_capability_roots( + &executor_roots, + &HashMap::from([("local".to_string(), manager.get_environment("local"))]), + ) + .await; + let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::default() + .snapshot(&resolved_roots, &Default::default()) .await; let catalog = provider .list(SkillListQuery { @@ -784,7 +864,7 @@ async fn pre_discovered_executor_catalog_snapshot() { include_cloud_skills: false, mcp_resources: None, executor_capability_discovery: Some(executor_capability_discovery), - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }) .await .expect("list pre-discovered executor skills"); @@ -902,7 +982,7 @@ async fn direct_executor_discovery_preserves_hidden_nested_and_probed_metadata() include_cloud_skills: false, mcp_resources: None, executor_capability_discovery: None, - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }) .await .expect("list directly discovered executor skills"); @@ -949,8 +1029,14 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), }, }]; - let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::new(manager) - .snapshot(&executor_roots, &Default::default()) + let resolved_roots = manager + .resolve_selected_capability_roots( + &executor_roots, + &HashMap::from([("local".to_string(), manager.get_environment("local"))]), + ) + .await; + let executor_capability_discovery = ExecutorCapabilityDiscoveryCache::default() + .snapshot(&resolved_roots, &Default::default()) .await; let catalog = provider .list(SkillListQuery { @@ -963,7 +1049,7 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { include_cloud_skills: false, mcp_resources: None, executor_capability_discovery: Some(executor_capability_discovery), - executor_capability_discovery_v2: Vec::new(), + executor_capability_discovery_v2: None, }) .await .expect("list executor skills"); @@ -991,7 +1077,7 @@ async fn high_level_discovery_reuses_materialized_skill_contents_for_reads() { async fn high_level_discovery_cache_separates_filesystem_permission_contexts() { let test_root = create_local_skill_root("permission-context").expect("create local skill root"); let manager = Arc::new(EnvironmentManager::default_for_tests()); - let cache = ExecutorCapabilityDiscoveryCache::new(manager); + let cache = ExecutorCapabilityDiscoveryCache::default(); let executor_roots = vec![SelectedCapabilityRoot { id: "permission-root".to_string(), location: CapabilityRootLocation::Environment { @@ -999,7 +1085,13 @@ async fn high_level_discovery_cache_separates_filesystem_permission_contexts() { path: PathUri::from_host_native_path(&test_root).expect("skill root URI"), }, }]; - cache.snapshot(&executor_roots, &HashMap::new()).await; + let resolved_roots = manager + .resolve_selected_capability_roots( + &executor_roots, + &HashMap::from([("local".to_string(), manager.get_environment("local"))]), + ) + .await; + cache.snapshot(&resolved_roots, &HashMap::new()).await; let updated_contents = "---\nname: synthetic\ndescription: Updated executor skill.\n---\n\nUPDATED_BODY\n"; @@ -1012,7 +1104,7 @@ async fn high_level_discovery_cache_separates_filesystem_permission_contexts() { PathUri::from_host_native_path(&test_root).expect("skill root URI"), ), )]); - let second_snapshot = cache.snapshot(&executor_roots, &sandbox_contexts).await; + let second_snapshot = cache.snapshot(&resolved_roots, &sandbox_contexts).await; let second_discovery = second_snapshot.roots()[0] .result .as_ref() @@ -1028,7 +1120,7 @@ async fn high_level_discovery_cache_separates_filesystem_permission_contexts() { "---\nname: synthetic\ndescription: Restored executor skill.\n---\n\nRESTORED_BODY\n"; std::fs::write(test_root.join("skill/SKILL.md"), restored_contents) .expect("restore executor skill"); - let restored_snapshot = cache.snapshot(&executor_roots, &HashMap::new()).await; + let restored_snapshot = cache.snapshot(&resolved_roots, &HashMap::new()).await; let restored_discovery = restored_snapshot.roots()[0] .result .as_ref() @@ -1046,7 +1138,7 @@ async fn high_level_discovery_cache_separates_filesystem_permission_contexts() { async fn high_level_discovery_batches_more_than_128_roots() { let test_root = create_local_skill_root("many-roots").expect("create local skill root"); let manager = Arc::new(EnvironmentManager::default_for_tests()); - let cache = ExecutorCapabilityDiscoveryCache::new(manager); + let cache = ExecutorCapabilityDiscoveryCache::default(); let executor_roots = (0..129) .map(|index| SelectedCapabilityRoot { id: format!("root-{index}"), @@ -1057,7 +1149,13 @@ async fn high_level_discovery_batches_more_than_128_roots() { }) .collect::>(); - let snapshot = cache.snapshot(&executor_roots, &HashMap::new()).await; + let resolved_roots = manager + .resolve_selected_capability_roots( + &executor_roots, + &HashMap::from([("local".to_string(), manager.get_environment("local"))]), + ) + .await; + let snapshot = cache.snapshot(&resolved_roots, &HashMap::new()).await; assert_eq!(snapshot.roots().len(), executor_roots.len()); assert!(snapshot.roots().iter().all(|root| root.result.is_ok())); diff --git a/codex-rs/ext/skills/tests/skills_extension.rs b/codex-rs/ext/skills/tests/skills_extension.rs index f08da69eb9..89fda9e0b0 100644 --- a/codex-rs/ext/skills/tests/skills_extension.rs +++ b/codex-rs/ext/skills/tests/skills_extension.rs @@ -1,4 +1,5 @@ use codex_protocol_test_support::ContentItemTestExt; +use std::collections::HashMap; use std::path::PathBuf; use std::sync::Arc; use std::sync::Mutex; @@ -12,7 +13,11 @@ use codex_config::ConfigLayerSource; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirementsToml; use codex_exec_server::CapabilityRootDiscovery; +use codex_exec_server::DiscoverV2CapabilitiesResponse; +use codex_exec_server::EnvironmentManager; use codex_exec_server::ExecutorCapabilityDiscoverySnapshot; +use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; +use codex_exec_server::ExecutorSkill; use codex_exec_server::FileSystemEnvironmentAccessor; use codex_exec_server::LOCAL_FS; use codex_extension_api::ConversationHistory; @@ -55,6 +60,7 @@ use codex_protocol::protocol::TruncationPolicy; use codex_protocol::protocol::TurnEnvironmentSelection; use codex_protocol::user_input::UserInput; use codex_skills::SkillMetadata; +use codex_skills_extension::ExecutorSkillProvider; use codex_skills_extension::HostSkillProvider; use codex_skills_extension::HostSkillsLoadInput; use codex_skills_extension::HostSkillsService; @@ -246,6 +252,7 @@ async fn installed_extension_uses_host_service_snapshot() -> TestResult { let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &ExtensionData::new("step"), turn_id: "turn-1".to_string(), user_input: vec![UserInput::Text { text: "$demo".to_string(), @@ -538,9 +545,16 @@ async fn persisted_host_snapshot_deduplicates_warning_after_reinitialization() - #[tokio::test] async fn executor_cloud_and_host_share_catalog_world_state_flow() -> TestResult { - let provider = |entry: SkillCatalogEntry| { + let provider = |mut entry: SkillCatalogEntry| { + if matches!( + entry.authority.kind, + SkillSourceKind::Cloud | SkillSourceKind::Host + ) { + entry.name = "shared-skill".to_string(); + } Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![entry], warnings: Vec::new(), }, @@ -556,12 +570,15 @@ async fn executor_cloud_and_host_share_catalog_world_state_flow() -> TestResult "executor/executor-skill", "skill://executor/executor-skill/SKILL.md", ))) - .with_cloud_provider(provider(test_entry( - SkillSourceKind::Cloud, - "codex_apps", - "cloud/cloud-skill", - "skill://cloud/cloud-skill/SKILL.md", - ))) + .with_cloud_provider(provider( + test_entry( + SkillSourceKind::Cloud, + "codex_apps", + "cloud/cloud-skill", + "skill://cloud/cloud-skill/SKILL.md", + ) + .with_remote_plugin_id(Some("shared-plugin".to_string())), + )) .with_host_provider(provider( test_entry( SkillSourceKind::Host, @@ -569,7 +586,8 @@ async fn executor_cloud_and_host_share_catalog_world_state_flow() -> TestResult "host/host-skill", "/skills/host-skill/SKILL.md", ) - .with_display_path("/skills/host-skill/SKILL.md"), + .with_display_path("/skills/host-skill/SKILL.md") + .with_remote_plugin_id(Some("shared-plugin".to_string())), )); let mut builder = ExtensionRegistryBuilder::new(); install_with_providers(&mut builder, providers, skills_extension_config); @@ -588,6 +606,7 @@ async fn executor_cloud_and_host_share_catalog_world_state_flow() -> TestResult thread_store: &thread_store, }) .await; + // Host rendering stays separate even when its identity matches a Cloud skill. start_registered_turn(®istry, &session_store, &thread_store, "turn-1").await; let selected_roots = [SelectedCapabilityRoot { @@ -636,11 +655,11 @@ async fn executor_cloud_and_host_share_catalog_world_state_flow() -> TestResult ), ( "cloud_skills", - "- cloud-skill: Fix lint errors. (cloud package: cloud/cloud-skill)", + "- shared-skill: Fix lint errors. (cloud package: cloud/cloud-skill)", ), ( "host_skills", - "- host-skill: Fix lint errors. (file: /skills/host-skill/SKILL.md)", + "- shared-skill: Fix lint errors. (file: /skills/host-skill/SKILL.md)", ), ] { let fragment = world_state_section(§ions, section_id) @@ -668,6 +687,7 @@ async fn executor_cloud_and_host_share_catalog_world_state_flow() -> TestResult async fn nonempty_executor_empty_host_records_catalog_metrics() -> TestResult { let executor_provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![test_entry( SkillSourceKind::Executor, "env-1", @@ -756,6 +776,7 @@ async fn host_world_state_uses_provider_catalog_with_core_compatible_rendering() let list_calls = Arc::new(AtomicUsize::new(0)); let provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![ test_entry( SkillSourceKind::Host, @@ -832,6 +853,7 @@ async fn shadow_selection_uses_host_catalog_when_instructions_are_disabled() -> let list_calls = Arc::new(AtomicUsize::new(0)); let provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![test_entry( SkillSourceKind::Host, "host", @@ -903,6 +925,7 @@ async fn shadow_selection_uses_host_catalog_when_instructions_are_disabled() -> let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &ExtensionData::new("step"), turn_id: "turn-1".to_string(), user_input: vec![UserInput::Text { text: "Fix lint errors.".to_string(), @@ -955,6 +978,7 @@ async fn shadow_selection_uses_host_catalog_when_instructions_are_disabled() -> async fn shadow_lru_selector_recovers_a_skill_invoked_on_an_earlier_turn() -> TestResult { let provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![test_entry( SkillSourceKind::Host, "host", @@ -1007,6 +1031,7 @@ async fn shadow_lru_selector_recovers_a_skill_invoked_on_an_earlier_turn() -> Te let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &ExtensionData::new("step"), turn_id: turn_id.to_string(), user_input: vec![UserInput::Text { text: text.to_string(), @@ -1112,7 +1137,9 @@ async fn shadow_lru_selector_recovers_a_skill_invoked_on_an_earlier_turn() -> Te } #[tokio::test] -async fn selected_executor_catalog_follows_step_availability_and_reuses_its_cache() -> TestResult { +async fn selected_executor_catalog_caches_root_selections_per_turn_and_reuses_discovery_snapshots() +-> TestResult { + let step_store = ExtensionData::new("step-1"); let read_requests = Arc::new(Mutex::new(Vec::new())); let list_calls = Arc::new(AtomicUsize::new(0)); let resource = SkillResourceId::environment( @@ -1129,6 +1156,7 @@ async fn selected_executor_catalog_follows_step_availability_and_reuses_its_cach entry.main_prompt = resource.clone(); let executor_provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![entry], warnings: Vec::new(), }, @@ -1192,7 +1220,7 @@ async fn selected_executor_catalog_follows_step_availability_and_reuses_its_cach session_store: &session_store, thread_store: &thread_store, turn_store: &turn_store, - step_store: &turn_store, + step_store: &step_store, }) .await; assert_eq!(1, available_sections.len()); @@ -1207,6 +1235,7 @@ async fn selected_executor_catalog_follows_step_availability_and_reuses_its_cach let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &step_store, turn_id: "turn-1".to_string(), user_input: vec![UserInput::Text { text: "$lint-fix please".to_string(), @@ -1312,7 +1341,34 @@ async fn selected_executor_catalog_follows_step_availability_and_reuses_its_cach .1, Some(available_fragment), ); - assert_eq!(1, list_calls.load(Ordering::Relaxed)); + assert_eq!(2, list_calls.load(Ordering::Relaxed)); + + // Retained steps keep their catalog even after the selected roots change. + let retained_sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + step_store: &step_store, + executor_capability_discovery_v2: None, + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &[], + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + assert_eq!( + retained_sections[0] + .render_diff(PreviousWorldStateSection::Absent) + .0 + .as_ref(), + Some(&available_snapshot) + ); + assert_eq!(2, list_calls.load(Ordering::Relaxed)); let failed_discovery = ExecutorCapabilityDiscoverySnapshot::new( &selected_roots, @@ -1333,9 +1389,9 @@ async fn selected_executor_catalog_follows_step_availability_and_reuses_its_cach Default::default(), ); for (turn_id, discovery, expected_list_calls) in [ - ("failed-discovery", &failed_discovery, 2), - ("recovered-discovery", &recovered_discovery, 3), - ("cached-discovery", &recovered_discovery, 3), + ("failed-discovery", &failed_discovery, 3), + ("recovered-discovery", &recovered_discovery, 4), + ("cached-discovery", &recovered_discovery, 4), ] { registry.context_contributors()[0] .contribute_world_state(WorldStateContributionInput { @@ -1405,6 +1461,79 @@ async fn selected_executor_catalog_follows_step_availability_and_reuses_its_cach .is_none() ); + let first_root = selected_roots[0].clone(); + let second_root = SelectedCapabilityRoot { + id: "second-root".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "env-1".to_string(), + path: PathUri::parse("file:///skills/second-root")?, + }, + }; + let cache_environment = Arc::new(codex_exec_server::Environment::create_for_tests( + /*exec_server_url*/ None, + )?); + let cache_manager = EnvironmentManager::default_for_tests(); + let previous_list_calls = list_calls.load(Ordering::Relaxed); + let mut current_config = listing_disabled_config; + for (turn_id, roots, include_bundled_skills, expected_new_calls) in [ + ("cache-turn-1", vec![first_root.clone()], true, 1), + ("cache-turn-1", vec![first_root.clone()], true, 1), + ( + "cache-turn-1", + vec![first_root.clone(), second_root.clone()], + true, + 2, + ), + ( + "cache-turn-1", + vec![second_root.clone(), first_root.clone()], + true, + 3, + ), + ("cache-turn-1", vec![first_root.clone()], true, 4), + ("cache-turn-2", vec![first_root.clone()], true, 5), + ("cache-turn-2", vec![first_root.clone()], false, 6), + ("cache-turn-2", vec![first_root], false, 6), + ] { + let mut next_config = current_config.clone(); + next_config.bundled_skills_enabled = include_bundled_skills; + registry.config_contributors()[0].on_config_changed( + &session_store, + &thread_store, + ¤t_config, + &next_config, + ); + current_config = next_config; + let cache_step = ExtensionData::new("cache-step"); + let captured = HashMap::from([("env-1".to_string(), Some(Arc::clone(&cache_environment)))]); + cache_step.insert( + cache_manager + .resolve_selected_capability_roots(&roots, &captured) + .await, + ); + registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + step_store: &cache_step, + executor_capability_discovery_v2: None, + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id, + environments: &[], + ready_selected_capability_roots: &roots, + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &ExtensionData::new(turn_id), + }) + .await; + assert_eq!( + previous_list_calls + expected_new_calls, + list_calls.load(Ordering::Relaxed) + ); + } + Ok(()) } @@ -1569,6 +1698,7 @@ async fn default_context_truncates_catalog_descriptions() -> TestResult { let providers = SkillProviders::new() .with_executor_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: executor_entries, warnings: Vec::new(), }, @@ -1578,6 +1708,7 @@ async fn default_context_truncates_catalog_descriptions() -> TestResult { })) .with_cloud_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![cloud_entry], warnings: Vec::new(), }, @@ -1677,6 +1808,7 @@ async fn catalog_rebalances_only_to_avoid_omissions_and_retains_the_allocation() let providers = SkillProviders::new() .with_executor_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: executor_entries.clone(), warnings: Vec::new(), }, @@ -1686,6 +1818,7 @@ async fn catalog_rebalances_only_to_avoid_omissions_and_retains_the_allocation() })) .with_cloud_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: cloud_entries, warnings: Vec::new(), }, @@ -1912,6 +2045,7 @@ async fn extreme_budget_pressure_removes_descriptions_before_omitting_entries() let providers = SkillProviders::new() .with_executor_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: executor_entries, warnings: Vec::new(), }, @@ -1921,6 +2055,7 @@ async fn extreme_budget_pressure_removes_descriptions_before_omitting_entries() })) .with_cloud_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: cloud_entries, warnings: Vec::new(), }, @@ -2018,6 +2153,7 @@ async fn skills_list_only_returns_model_visible_bounded_metadata() -> TestResult final_entry.description = "d".repeat(50); let providers = SkillProviders::new().with_cloud_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![ entry, first_entry, @@ -2063,7 +2199,26 @@ async fn skills_list_only_returns_model_visible_bounded_metadata() -> TestResult .await; start_registered_turn(®istry, &session_store, &thread_store, "turn-1").await; - let tools = registry.tool_contributors()[0].tools(&session_store, &thread_store); + let step_store = ExtensionData::new("step"); + registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + step_store: &step_store, + executor_capability_discovery_v2: None, + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &[], + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &ExtensionData::new("turn-1"), + }) + .await; + let tools = + registry.tool_contributors()[0].tools_for_step(&session_store, &thread_store, &step_store); let list_tool = tools .iter() .find(|tool| tool.tool_name().name == "list") @@ -2262,14 +2417,18 @@ async fn skills_list_only_returns_model_visible_bounded_metadata() -> TestResult #[tokio::test] async fn cloud_discovery_is_gated_and_reports_failures() -> TestResult { - for cloud_skill_enabled in [false, true] { + for (cloud_skill_enabled, has_cloud_provider) in [(false, true), (true, true), (true, false)] { let list_calls = Arc::new(AtomicUsize::new(0)); - let providers = SkillProviders::new().with_cloud_provider(Arc::new(StaticSkillProvider { - catalog: SkillCatalog::default(), - read_requests: Arc::new(Mutex::new(Vec::new())), - list_calls: Some(Arc::clone(&list_calls)), - fail_first_list: true, - })); + let mut providers = SkillProviders::new(); + if has_cloud_provider { + providers = providers.with_cloud_provider(Arc::new(StaticSkillProvider { + catalog: SkillCatalog::default(), + read_requests: Arc::new(Mutex::new(Vec::new())), + list_calls: Some(Arc::clone(&list_calls)), + fail_first_list: true, + })); + } + let cloud_available = cloud_skill_enabled && has_cloud_provider; let (event_tx, event_rx) = std::sync::mpsc::channel(); let mut builder = ExtensionRegistryBuilder::with_event_sink(Arc::new(ChannelEventSink(event_tx))); @@ -2303,7 +2462,7 @@ async fn cloud_discovery_is_gated_and_reports_failures() -> TestResult { .collect::>(), vec![ codex_extension_api::TurnStartPhase::BeforeTaskRegistration, - if cloud_skill_enabled { + if cloud_available { codex_extension_api::TurnStartPhase::RegularTaskStart } else { codex_extension_api::TurnStartPhase::BeforeTaskRegistration @@ -2316,16 +2475,61 @@ async fn cloud_discovery_is_gated_and_reports_failures() -> TestResult { .iter() .map(|contributor| contributor.requires_mcp_runtime(&thread_store)) .collect::>(), - vec![false, cloud_skill_enabled] + vec![false, cloud_available] ); start_registered_turn(®istry, &session_store, &thread_store, "turn-1").await; assert_eq!( list_calls.load(Ordering::Relaxed), - usize::from(cloud_skill_enabled) + usize::from(cloud_available) ); - if !cloud_skill_enabled { + for discovery_v2 in [None, Some([].as_slice())] { + let sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + step_store: &ExtensionData::new("step"), + executor_capability_discovery_v2: discovery_v2, + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &[], + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &ExtensionData::new("turn-1"), + }) + .await; + if has_cloud_provider { + // Resume may retain a catalog even when the new Cloud discovery fails. + let previous = serde_json::json!({ + "body": "Previously advertised Cloud skills", + "includeInstructions": true, + "enabled": true, + }); + let update = world_state_section(§ions, "cloud_skills") + .render_diff(PreviousWorldStateSection::Known(&previous)) + .1 + .ok_or("unavailable Cloud skills should withdraw the previous catalog")?; + assert_eq!( + update.body().clone().expect_text(), + "\n## Cloud skills update\nNo cloud skills are currently available.\n" + ); + } else { + assert!( + sections + .iter() + .all(|section| section.id() != "cloud_skills") + ); + } + } + if !cloud_available { assert!(event_rx.try_recv().is_err()); - let tools = registry.tool_contributors()[0].tools(&session_store, &thread_store); + let tools = registry.tool_contributors()[0].tools_for_step( + &session_store, + &thread_store, + &ExtensionData::new("step"), + ); assert!(tools.iter().all(|tool| tool.tool_name().name != "list")); continue; } @@ -2337,14 +2541,180 @@ async fn cloud_discovery_is_gated_and_reports_failures() -> TestResult { Ok(()) } +#[tokio::test] +async fn v2_read_cache_is_scoped_to_the_captured_environment() -> TestResult { + let directory = tempfile::tempdir()?; + let path = directory.path().join("SKILL.md"); + std::fs::write(&path, "original\n".repeat(2_000))?; + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let environment = manager + .get_environment("local") + .ok_or("local environment")?; + let replacement = EnvironmentManager::default_for_tests() + .get_environment("local") + .ok_or("replacement environment")?; + let providers = SkillProviders::new().with_executor_provider(Arc::new( + ExecutorSkillProvider::new_with_restriction_product( + manager, /*restriction_product*/ None, + ), + )); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers, skills_extension_config); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &default_config(), + session_source: &SessionSource::Cli, + persistent_thread_state_available: true, + environments: &[], + mcp_resource_client: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + let snapshots = vec![ExecutorCapabilityDiscoveryV2Snapshot { + environment_id: "captured".to_string(), + result: Ok(Arc::new(DiscoverV2CapabilitiesResponse { + skills: vec![ExecutorSkill { + name: "demo".to_string(), + description: "Demo".to_string(), + short_description: None, + path: PathUri::from_host_native_path(&path)?, + discovery_root: None, + dependencies: None, + policy: None, + }], + ..Default::default() + })), + }]; + let package = format!( + "skill://skills/{}", + PathUri::from_host_native_path(directory.path())? + .inferred_native_path_string() + .replace('\\', "/") + .trim_start_matches('/') + ); + let turn_store = ExtensionData::new("turn-1"); + let mut cursor = None; + for (handle, expected_error) in [ + (Some(Arc::clone(&environment)), None), + (Some(environment), None), + ( + Some(replacement), + Some("skills.read cursor is stale; restart from the first page"), + ), + ( + None, + Some("skill environment is not available for this callback"), + ), + ] { + let file_system = handle.as_ref().map(|handle| handle.get_filesystem()); + let access = file_system + .as_ref() + .map(FileSystemEnvironmentAccessor::unrestricted); + let step_store = ExtensionData::new("turn-1"); + step_store.insert(snapshots.clone()); + step_store.insert( + handle + .map(|handle| ("captured".to_string(), handle)) + .into_iter() + .collect::>(), + ); + registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + step_store: &step_store, + executor_capability_discovery_v2: Some(&snapshots), + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &[], + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + let tools = registry.tool_contributors()[0].tools_for_step( + &session_store, + &thread_store, + &step_store, + ); + let read = tools + .iter() + .find(|tool| tool.tool_name().name == "read") + .ok_or("skills.read")?; + let payload = ToolPayload::Function { + arguments: serde_json::json!({"package": package, "cursor": cursor}).to_string(), + }; + let result = read + .handle(ToolCall { + turn_id: "turn-1".to_string(), + call_id: "read".to_string(), + tool_name: read.tool_name(), + model: "gpt-test".to_string(), + codex_turn_metadata: None, + truncation_policy: TruncationPolicy::Bytes(2_000), + source: ToolCallSource::Direct, + conversation_history: ConversationHistory::default(), + turn_item_emitter: Arc::new(NoopTurnItemEmitter), + attachment_store: Arc::new(InlineAttachmentStore), + ephemeral: false, + environments: access + .iter() + .map(|access| { + codex_extension_api::ToolEnvironment::new( + "captured".to_string(), + PathUri::from_host_native_path(directory.path()).expect("fixture cwd"), + access, + ) + }) + .collect(), + payload: payload.clone(), + }) + .await; + if let Some(expected) = expected_error { + assert!( + matches!(result, Err(FunctionCallError::RespondToModel(message)) if message == expected) + ); + } else { + let response = result? + .post_tool_use_response("read", &payload) + .ok_or("read output")?; + assert!( + response["contents"] + .as_str() + .ok_or("contents")? + .contains("original") + ); + cursor = Some( + response["next_cursor"] + .as_str() + .ok_or("next cursor")? + .to_string(), + ); + // The next page keeps the cached contents only while the captured handle is unchanged. + std::fs::write(&path, "updated\n".repeat(2_000))?; + } + } + Ok(()) +} + #[tokio::test] async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> TestResult { + let step_store = ExtensionData::new("step-1"); let read_requests = Arc::new(Mutex::new(Vec::new())); let root_a_locator = "skill://root-a/shared/lint-fix/SKILL.md"; let root_b_locator = "skill://root-b/shared/lint-fix/SKILL.md"; let skill_path = PathUri::parse("file:///shared/lint-fix/SKILL.md")?; let executor_provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: [("root-a", root_a_locator), ("root-b", root_b_locator)] .into_iter() .map(|(root_id, locator)| { @@ -2416,12 +2786,13 @@ async fn root_qualified_locator_selects_only_the_matching_executor_skill() -> Te session_store: &session_store, thread_store: &thread_store, turn_store: &turn_store, - step_store: &turn_store, + step_store: &step_store, }) .await; let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &step_store, turn_id: "turn-1".to_string(), user_input: vec![UserInput::Mention { name: "lint-fix".to_string(), @@ -2480,6 +2851,7 @@ async fn model_context_window_scales_executor_and_cloud_catalogs() -> TestResult let providers = SkillProviders::new() .with_cloud_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: cloud_entries, warnings: Vec::new(), }, @@ -2489,6 +2861,7 @@ async fn model_context_window_scales_executor_and_cloud_catalogs() -> TestResult })) .with_executor_provider(Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: executor_entries, warnings: Vec::new(), }, @@ -2625,8 +2998,10 @@ async fn model_context_window_scales_executor_and_cloud_catalogs() -> TestResult #[tokio::test] async fn executor_catalog_emits_at_most_four_warnings() -> TestResult { + let step_store = ExtensionData::new("step-1"); let executor_provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: Vec::new(), warnings: (0..6).map(|index| format!("warning-{index}")).collect(), }, @@ -2679,12 +3054,13 @@ async fn executor_catalog_emits_at_most_four_warnings() -> TestResult { session_store: &session_store, thread_store: &thread_store, turn_store: &turn_store, - step_store: &turn_store, + step_store: &step_store, }) .await; registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &step_store, turn_id: "turn-1".to_string(), user_input: Vec::new(), environments: Vec::new(), @@ -2791,6 +3167,7 @@ async fn host_catalog_compacts_shared_paths_under_budget_pressure() -> TestResul let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &ExtensionData::new("step"), turn_id: "turn-1".to_string(), user_input: vec![UserInput::Text { text: "hello".to_string(), @@ -2830,6 +3207,7 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { let read_requests = Arc::new(Mutex::new(Vec::new())); let provider = Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![ test_entry( SkillSourceKind::Host, @@ -2875,6 +3253,7 @@ async fn prompt_hidden_skill_can_still_be_invoked() -> TestResult { let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &ExtensionData::new("step"), turn_id: "turn-1".to_string(), user_input: vec![UserInput::Text { text: "$hidden-skill".to_string(), @@ -3122,3 +3501,121 @@ fn read_request_keys( .unwrap_or_else(std::sync::PoisonError::into_inner) .clone() } + +#[path = "skills_extension/merge_catalog_tests.rs"] +mod merge_catalog_tests; + +#[path = "skills_extension/discovery_v2_tests.rs"] +mod discovery_v2_tests; + +#[tokio::test] +async fn shared_catalog_keeps_captured_executor_and_separates_bindings_in_one_turn() -> TestResult { + let root_dir = tempfile::tempdir()?; + let skill_dir = root_dir.path().join("demo"); + std::fs::create_dir_all(&skill_dir)?; + std::fs::write(skill_dir.join("SKILL.md"), DEMO_SKILL_CONTENTS)?; + let manager = Arc::new(EnvironmentManager::default_for_tests()); + let original = manager.get_environment("local").ok_or("local executor")?; + let replacement = EnvironmentManager::default_for_tests() + .get_environment("local") + .ok_or("replacement")?; + let roots = vec![SelectedCapabilityRoot { + id: "skills".into(), + location: CapabilityRootLocation::Environment { + environment_id: "captured".into(), + path: PathUri::from_host_native_path(root_dir.path())?, + }, + }]; + let captured = manager + .resolve_selected_capability_roots( + &roots, + &HashMap::from([("captured".into(), Some(original))]), + ) + .await; + let newer = manager + .resolve_selected_capability_roots( + &roots, + &HashMap::from([("captured".into(), Some(replacement))]), + ) + .await; + let listener = std::net::TcpListener::bind("127.0.0.1:0")?; + let address = listener.local_addr()?; + drop(listener); + manager.upsert_environment( + "captured".into(), + format!("ws://{address}"), + Some(std::time::Duration::from_millis(100)), + )?; + let provider = Arc::new(ExecutorSkillProvider::new_with_restriction_product( + manager, /*restriction_product*/ None, + )); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers( + &mut builder, + SkillProviders::new().with_executor_provider(provider), + skills_extension_config, + ); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + let turn_store = ExtensionData::new("same-turn"); + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &default_config(), + session_source: &SessionSource::Cli, + persistent_thread_state_available: true, + environments: &[], + mcp_resource_client: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + for (index, (resolved, expected)) in [ + (captured.clone(), Some("Demo skill.")), + (captured, Some("Demo skill.")), + (newer, Some("New executor inventory.")), + (Vec::new(), None), + ] + .into_iter() + .enumerate() + { + if index == 1 { + std::fs::write( + skill_dir.join("SKILL.md"), + "---\nname: demo\ndescription: New executor inventory.\n---\nNew body\n", + )?; + } + let step_store = ExtensionData::new(format!("step-{index}")); + step_store.insert(resolved); + let sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + executor_capability_discovery_v2: None, + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id: "same-turn", + environments: &[], + ready_selected_capability_roots: &roots, + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + step_store: &step_store, + }) + .await; + let rendered = sections + .iter() + .filter_map(|section| section.render_diff(PreviousWorldStateSection::Absent).1) + .map(|fragment| fragment.body().clone().expect_text()) + .collect::>() + .join("\n"); + if let Some(expected) = expected { + assert!(rendered.contains(expected), "step {index}: {rendered}"); + } else { + assert!(!rendered.contains("- demo:"), "{rendered}"); + } + } + Ok(()) +} diff --git a/codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs b/codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs new file mode 100644 index 0000000000..6b49c6b42f --- /dev/null +++ b/codex-rs/ext/skills/tests/skills_extension/discovery_v2_tests.rs @@ -0,0 +1,280 @@ +//! Verifies metadata-only V2 skill projection and refresh without legacy discovery. + +use super::*; +use codex_exec_server::DiscoverV2CapabilitiesResponse; +use codex_exec_server::DiscoveredPluginFiles; +use codex_exec_server::EnvironmentManager; +use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; +use codex_exec_server::ExecutorPlugin; +use codex_exec_server::ExecutorSkill; +use codex_skills::SkillDependencies; +use codex_skills::SkillPolicy; +use codex_skills::SkillToolDependency; +use codex_skills_extension::ExecutorSkillProvider; +use codex_skills_extension::merge_catalogs; +use pretty_assertions::assert_eq; + +fn installation( + environment_id: &str, +) -> Result> { + Ok(ExecutorCapabilityDiscoveryV2Snapshot { + environment_id: environment_id.to_string(), + result: Ok(Arc::new(DiscoverV2CapabilitiesResponse { + plugins: vec![ExecutorPlugin { + id: "local-name@marketplace".to_string(), + display_name: "Local plugin".to_string(), + remote_plugin_id: Some("Plugin_123".to_string()), + version: "1".to_string(), + root: PathUri::parse("file:///plugins/local-name/1")?, + files: DiscoveredPluginFiles { + manifest: codex_exec_server::CapabilityTextFile { + path: PathUri::parse( + "file:///plugins/local-name/1/.codex-plugin/plugin.json", + )?, + contents: r#"{"name":"local-name"}"#.to_string(), + }, + mcp_config: None, + apps_config: None, + }, + skills: vec![ExecutorSkill { + name: "local-name:work".to_string(), + description: "Environment description".to_string(), + short_description: None, + path: PathUri::parse( + "file:///plugins/local-name/1/skills/other-directory/SKILL.md", + )?, + discovery_root: None, + dependencies: None, + policy: None, + }], + }], + ..Default::default() + })), + }) +} + +#[tokio::test] +async fn v2_steps_refresh_metadata_and_preserve_separate_environments() -> TestResult { + let provider = Arc::new(ExecutorSkillProvider::new_with_restriction_product( + Arc::new(EnvironmentManager::default_for_tests()), + /*restriction_product*/ None, + )); + let dependencies = Some(SkillDependencies { + tools: vec![SkillToolDependency { + r#type: "mcp".into(), + value: "deploy".into(), + description: None, + transport: Some("stdio".into()), + command: Some("deploy-mcp".into()), + url: None, + oauth_callback_port: None, + }], + }); + let skill_path = PathUri::parse("file:///executor/skills/dashboard/SKILL.md")?; + let mut snapshots = vec![installation("remote-a")?, installation("remote-b")?]; + for snapshot in &mut snapshots { + Arc::make_mut(snapshot.result.as_mut().unwrap()) + .skills + .push(ExecutorSkill { + name: "dashboard".to_string(), + description: "Build a dashboard".to_string(), + short_description: None, + path: skill_path.clone(), + discovery_root: None, + dependencies: dependencies.clone(), + policy: Some(SkillPolicy { + allow_implicit_invocation: Some(false), + products: vec![], + }), + }); + } + let query = SkillListQuery { + turn_id: "turn-1".to_string(), + executor_roots: Vec::new(), + resolved_executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: false, + include_cloud_skills: false, + mcp_resources: None, + executor_capability_discovery: None, + executor_capability_discovery_v2: Some(snapshots.clone()), + }; + let catalog = provider.list(query).await?; + assert_eq!(catalog.entries.len(), 4); + let merged = merge_catalogs(/*cloud*/ None, catalog.clone()); + assert_eq!(merged, catalog); + for entries in [ + (&merged.entries[0], &merged.entries[2]), + (&merged.entries[1], &merged.entries[3]), + ] { + let (first, second) = entries; + assert_eq!(first.main_prompt.environment_id(), Some("remote-a")); + assert_eq!(second.main_prompt.environment_id(), Some("remote-b")); + assert_eq!(first.authority, second.authority); + assert_eq!(first.id, second.id); + assert_ne!(first.main_prompt, second.main_prompt); + assert!(first.sources.is_empty()); + assert!(second.sources.is_empty()); + } + + let mut rebound = merged.entries[0].clone(); + rebound.main_prompt = merged.entries[2].main_prompt.clone(); + let mut deduplicated = SkillCatalog::default(); + deduplicated.push_entry(merged.entries[0].clone()); + deduplicated.push_entry(rebound); + deduplicated.push_entry(merged.entries[2].clone()); + assert_eq!( + deduplicated.entries, + vec![merged.entries[0].clone(), merged.entries[2].clone()] + ); + + let plugin = merged + .entries + .iter() + .find(|entry| entry.plugin_id.is_some()) + .unwrap(); + assert_eq!(plugin.authority.id, "local-name@marketplace"); + assert_eq!( + plugin.id.0, + "skill://local-name@marketplace/plugins/local-name/1/skills/other-directory" + ); + + let entry = merged + .entries + .iter() + .find(|entry| entry.plugin_id.is_none()) + .unwrap(); + assert_eq!(entry.dependencies, dependencies); + assert!(entry.enabled); + assert!(!entry.prompt_visible); + assert_eq!(entry.authority.id, "skills"); + assert_eq!(entry.id.0, "skill://skills/executor/skills/dashboard"); + assert_eq!( + entry.main_prompt, + SkillResourceId::environment(entry.main_prompt.as_str(), "remote-a", skill_path.clone(),) + ); + let providers = SkillProviders::new().with_executor_provider(provider); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers(&mut builder, providers, skills_extension_config); + let registry = builder.build(); + let session_store = ExtensionData::new("session"); + let thread_store = ExtensionData::new("thread"); + registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &default_config(), + session_source: &SessionSource::Cli, + persistent_thread_state_available: true, + environments: &[], + mcp_resource_client: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + }) + .await; + let turn_store = ExtensionData::new("turn-1"); + for description in [Some("original"), Some("updated"), None] { + if let Some(description) = description { + for snapshot in &mut snapshots { + Arc::make_mut(snapshot.result.as_mut().unwrap()).plugins[0].skills[0].description = + description.to_string(); + } + } else { + snapshots.clear(); + } + let sections = registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + step_store: &ExtensionData::new("step"), + executor_capability_discovery_v2: Some(&snapshots), + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id: "turn-1", + environments: &[], + ready_selected_capability_roots: &[], + executor_capability_discovery: None, + extension_metrics: None, + session_store: &session_store, + thread_store: &thread_store, + turn_store: &turn_store, + }) + .await; + let rendered = sections + .iter() + .filter_map(|section| section.render_diff(PreviousWorldStateSection::Absent).1) + .map(|fragment| fragment.body().clone().expect_text()) + .collect::>() + .join("\n"); + if let Some(description) = description { + assert!( + rendered.contains("skill://local-name@marketplace/"), + "{rendered}" + ); + assert!( + rendered.contains(&format!("- local-name:work: {description}")), + "{rendered}" + ); + assert!(!rendered.contains("- dashboard:"), "{rendered}"); + } else { + assert!(!rendered.contains("- local-name:work:"), "{rendered}"); + } + } + Ok(()) +} + +#[tokio::test] +async fn v2_empty_failed_and_partial_results_never_load_legacy_roots() -> TestResult { + let provider = ExecutorSkillProvider::new_with_restriction_product( + Arc::new(EnvironmentManager::default_for_tests()), + /*restriction_product*/ None, + ); + let successful = installation("executor-a")?; + let failed = ExecutorCapabilityDiscoveryV2Snapshot { + environment_id: "executor-b".to_string(), + result: Err("discovery unavailable".to_string()), + }; + let query = |snapshots| SkillListQuery { + turn_id: "turn-1".to_string(), + // A legacy lookup would report an unavailable environment. V2 must ignore this root. + executor_roots: vec![SelectedCapabilityRoot { + id: "legacy".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "unavailable-legacy-executor".to_string(), + path: PathUri::parse("file:///legacy").unwrap(), + }, + }], + resolved_executor_roots: Vec::new(), + host_snapshot: None, + include_host_skills: false, + include_bundled_skills: false, + include_cloud_skills: false, + mcp_resources: None, + executor_capability_discovery: None, + executor_capability_discovery_v2: Some(snapshots), + }; + let successful_catalog = provider.list(query(vec![successful.clone()])).await?; + let failure_warning = + "Environment `executor-b` discovery failed: discovery unavailable".to_string(); + for (snapshots, expected) in [ + (Vec::new(), SkillCatalog::default()), + ( + vec![failed.clone()], + SkillCatalog { + is_partial: true, + entries: Vec::new(), + warnings: vec![failure_warning.clone()], + }, + ), + ( + vec![successful, failed], + SkillCatalog { + is_partial: true, + warnings: vec![failure_warning], + ..successful_catalog + }, + ), + ] { + assert_eq!(provider.list(query(snapshots)).await?, expected); + } + Ok(()) +} diff --git a/codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs b/codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs new file mode 100644 index 0000000000..7720929836 --- /dev/null +++ b/codex-rs/ext/skills/tests/skills_extension/merge_catalog_tests.rs @@ -0,0 +1,780 @@ +//! Exercises V2 catalog merging, retained read routes, and unchanged legacy rendering. + +use super::*; +use codex_core_plugin_common::PluginIdentity; +use codex_exec_server::DiscoverV2CapabilitiesResponse; +use codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot; +use codex_extension_api::SelectedPluginIdentity; +use codex_extension_api::SelectedPluginSnapshot; +use codex_extension_api::ToolEnvironment; +use codex_extension_api::ToolExecutor; +use codex_skills_extension::detect_implicit_skill_invocation; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; + +fn cloud_skill(name: &str) -> SkillCatalogEntry { + let package = format!("skill://Plugin_123/{name}"); + SkillCatalogEntry::new( + SkillPackageId(package.clone()), + SkillAuthority::new(SkillSourceKind::Cloud, "codex_apps"), + format!("deploy:{name}"), + format!("Cloud {name} description"), + SkillResourceId::new(format!("{package}/SKILL.md")), + ) + .with_remote_plugin_id(Some("Plugin_123".to_string())) +} + +fn installed_skill( + root: &str, + name: &str, +) -> Result> { + let package = format!("skill://{root}/{name}"); + Ok(SkillCatalogEntry::new( + SkillPackageId(package.clone()), + SkillAuthority::new(SkillSourceKind::Executor, root), + format!("deploy:{name}"), + format!("Installed {root} description"), + SkillResourceId::environment( + format!("{package}/SKILL.md"), + "env-1", + PathUri::parse(&format!("file:///skills/{root}/SKILL.md"))?, + ), + ) + .with_display_path(format!("{package}/SKILL.md")) + .with_remote_plugin_id(Some("Plugin_123".to_string()))) +} + +fn provider(entries: Vec) -> StaticSkillProvider { + StaticSkillProvider { + catalog: SkillCatalog { + is_partial: false, + entries, + warnings: Vec::new(), + }, + read_requests: Arc::default(), + list_calls: Some(Arc::default()), + fail_first_list: false, + } +} + +struct MergeHarness { + registry: codex_extension_api::ExtensionRegistry, + session_store: ExtensionData, + thread_store: ExtensionData, + turn_store: ExtensionData, + step_store: ExtensionData, + roots: Vec, + snapshots: Option>, + discovery: Option, +} + +impl MergeHarness { + async fn new( + executor: StaticSkillProvider, + cloud: StaticSkillProvider, + config: TestConfig, + ) -> Result> { + Self::with_metrics(executor, cloud, config, /*metrics*/ None).await + } + + async fn with_metrics( + executor: StaticSkillProvider, + cloud: StaticSkillProvider, + config: TestConfig, + metrics: Option, + ) -> Result> { + let providers = SkillProviders::new() + .with_executor_provider(Arc::new(executor)) + .with_cloud_provider(Arc::new(cloud)); + let mut builder = ExtensionRegistryBuilder::new(); + install_with_providers_and_metrics( + &mut builder, + providers, + metrics, + skills_extension_config, + ); + let harness = Self { + registry: builder.build(), + session_store: ExtensionData::new("session"), + thread_store: ExtensionData::new("thread"), + turn_store: ExtensionData::new("turn-1"), + step_store: ExtensionData::new("step-1"), + discovery: None, + snapshots: Some(vec![ExecutorCapabilityDiscoveryV2Snapshot { + environment_id: "env-1".to_string(), + result: Ok(Arc::new(DiscoverV2CapabilitiesResponse::default())), + }]), + roots: vec![SelectedCapabilityRoot { + id: "skills".to_string(), + location: CapabilityRootLocation::Environment { + environment_id: "env-1".to_string(), + path: PathUri::parse("file:///skills")?, + }, + }], + }; + harness.registry.thread_lifecycle_contributors()[0] + .on_thread_start(ThreadStartInput { + config: &config, + session_source: &SessionSource::Cli, + persistent_thread_state_available: true, + environments: &[], + mcp_resource_client: None, + extension_metrics: None, + session_store: &harness.session_store, + thread_store: &harness.thread_store, + }) + .await; + start_registered_turn( + &harness.registry, + &harness.session_store, + &harness.thread_store, + "turn-1", + ) + .await; + Ok(harness) + } + + fn tools( + &self, + step: &ExtensionData, + ) -> Vec ToolExecutor>>> { + if let Some(snapshots) = &self.snapshots { + step.insert(snapshots.clone()); + } + if let Some(discovery) = &self.discovery { + step.insert(discovery.clone()); + } + self.registry.tool_contributors()[0].tools_for_step( + &self.session_store, + &self.thread_store, + step, + ) + } + + async fn render(&self) -> Result> { + self.render_step(&self.step_store).await + } + + async fn render_step( + &self, + step_store: &ExtensionData, + ) -> Result> { + let turn_store = &self.turn_store; + if let Some(plugins) = turn_store.get::() { + step_store.insert(plugins.as_ref().clone()); + } + let sections = self.registry.context_contributors()[0] + .contribute_world_state(WorldStateContributionInput { + previous_world_state: None, + step_store, + model_info: &catalog_model_info(), + thread_id: codex_protocol::ThreadId::new(), + turn_id: turn_store.level_id(), + environments: &[], + ready_selected_capability_roots: &self.roots, + executor_capability_discovery: self.discovery.as_ref(), + executor_capability_discovery_v2: self.snapshots.as_deref(), + extension_metrics: None, + session_store: &self.session_store, + thread_store: &self.thread_store, + turn_store, + }) + .await; + Ok(sections + .iter() + .filter_map(|section| section.render_diff(PreviousWorldStateSection::Absent).1) + .map(|fragment| fragment.body().clone().expect_text()) + .collect::>() + .join("\n")) + } +} + +async fn call_tool( + tools: &[Arc ToolExecutor>>], + name: &str, + arguments: Value, +) -> Result { + let tool = tools + .iter() + .find(|tool| tool.tool_name().name == name) + .ok_or_else(|| FunctionCallError::RespondToModel("missing skill tool".to_string()))?; + let payload = ToolPayload::Function { + arguments: arguments.to_string(), + }; + let access = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS); + let result = tool + .handle(ToolCall { + turn_id: "turn-1".to_string(), + call_id: "skill-test".to_string(), + tool_name: tool.tool_name(), + model: "test".to_string(), + codex_turn_metadata: None, + truncation_policy: TruncationPolicy::Bytes(10_000), + source: ToolCallSource::Direct, + conversation_history: ConversationHistory::default(), + turn_item_emitter: Arc::new(NoopTurnItemEmitter), + attachment_store: Arc::new(InlineAttachmentStore), + ephemeral: false, + environments: vec![ToolEnvironment::new( + "env-1".to_string(), + PathUri::parse("file:///installed") + .map_err(|error| FunctionCallError::RespondToModel(error.to_string()))?, + &access, + )], + payload: payload.clone(), + }) + .await?; + if name == "list" { + assert_eq!( + result.contains_external_context(), + arguments["authority"]["kind"] == "cloud" + ); + } + result + .post_tool_use_response("skill-test", &payload) + .ok_or_else(|| FunctionCallError::RespondToModel("missing skill response".to_string())) +} + +fn selected_plugins(roots: &[Option<&str>], disabled: &[&str]) -> SelectedPluginSnapshot { + SelectedPluginSnapshot { + plugins: roots + .iter() + .map(|root| SelectedPluginIdentity { + selected_root_id: (*root).map(str::to_string), + plugin_id: PluginIdentity::Remote { + remote_plugin_id: "Plugin_123".to_string(), + }, + }) + .collect(), + disabled_plugin_ids: disabled.iter().map(|id| (*id).to_string()).collect(), + ..Default::default() + } +} + +#[tokio::test] +async fn v2_merged_skills_preserve_cloud_metadata_and_accepted_plugin_routes() -> TestResult { + let cloud = cloud_skill("work"); + let mut installed = Vec::new(); + for (id, name) in [ + ("matching", "work"), + ("removed", "removed"), + ("unlisted", "work"), + ("denied", "work"), + ] { + installed.push(installed_skill(id, name)?.with_plugin_id(Some(id.to_string()))); + } + let local = installed_skill("local", "local")? + .with_remote_plugin_id(/*remote_plugin_id*/ None) + .with_plugin_id(Some("local".to_string())); + installed.push(local.clone()); + let standalone_cloud = + cloud_skill("cloud").with_remote_plugin_id(/*remote_plugin_id*/ None); + let standalone_executor = installed_skill("standalone", "executor")? + .with_remote_plugin_id(/*remote_plugin_id*/ None); + installed.push(standalone_executor.clone()); + let mut executor_provider = provider(installed.clone()); + executor_provider.catalog.warnings = + vec!["Executor A warning".into(), "Executor B warning".into()]; + let mut cloud_provider = provider(vec![cloud.clone(), standalone_cloud.clone()]); + cloud_provider.catalog.warnings = vec!["Cloud warning".into()]; + let harness = MergeHarness::new(executor_provider, cloud_provider, default_config()).await?; + let mut selection = selected_plugins( + &[ + None, + Some("env-1:matching"), + Some("env-1:removed"), + Some("env-1:denied"), + Some("other:unlisted"), + ], + &["denied"], + ); + selection.plugins.push(SelectedPluginIdentity { + selected_root_id: Some("env-1:local".to_string()), + plugin_id: PluginIdentity::Local { + plugin_id: "local".to_string(), + }, + }); + harness.turn_store.insert(selection); + let tools = harness.tools(&harness.step_store); + let rendered = harness.render().await?; + assert_eq!(rendered.matches("- deploy:work:").count(), 1); + assert!(rendered.contains("Cloud work description")); + assert!(!rendered.contains("Installed matching description")); + assert!(rendered.contains("Installed local description")); + assert!(rendered.contains("- deploy:cloud:")); + assert!(rendered.contains("- deploy:executor:")); + for (authority, expected_packages, warnings) in [ + ( + "cloud", + vec![&standalone_cloud.id, &cloud.id], + json!(["Cloud warning"]), + ), + ( + "executor", + vec![&local.id, &standalone_executor.id], + json!(["Executor A warning", "Executor B warning"]), + ), + ] { + let output = call_tool(&tools, "list", json!({"authority": {"kind": authority}})).await?; + let packages = output["skills"] + .as_array() + .unwrap() + .iter() + .map(|skill| skill["package"].clone()) + .collect::>(); + assert_eq!( + packages, + expected_packages + .iter() + .map(|package| json!(package.0)) + .collect::>() + ); + assert_eq!(output["warnings"], warnings); + assert!( + output["skills"] + .as_array() + .unwrap() + .iter() + .all(|skill| skill["authority"]["kind"] == authority) + ); + } + call_tool(&tools, "read", json!({"package": cloud.id.0})).await?; + // Unselected, disabled, and Cloud-absent installations cannot bypass the merge. + for entry in &installed { + let allowed = matches!( + entry.authority.id.as_str(), + "matching" | "local" | "standalone" + ); + let result = call_tool(&tools, "read", json!({"package": entry.id.0})).await; + if allowed { + result?; + } else { + assert_matches!(result, Err(FunctionCallError::RespondToModel(message)) if message == "skill package is not available"); + } + } + Ok(()) +} + +#[tokio::test] +async fn v2_only_complete_cloud_catalog_excludes_unmatched_remote_skills() -> TestResult { + let mut unavailable = provider(vec![cloud_skill("work")]); + unavailable.fail_first_list = true; + let mut partial = provider(Vec::new()); + partial.catalog.is_partial = true; + for (cloud, config, keeps_remote) in [ + ( + provider(vec![cloud_skill("work")]), + TestConfig { + cloud_skill_enabled: false, + ..default_config() + }, + true, + ), + (unavailable, default_config(), true), + (partial, default_config(), true), + (provider(Vec::new()), default_config(), false), + ] { + let harness = MergeHarness::new( + provider(vec![ + installed_skill("matching", "work")?.with_plugin_id(Some("matching".to_string())), + installed_skill("standalone", "local")? + .with_remote_plugin_id(/*remote_plugin_id*/ None), + ]), + cloud, + config, + ) + .await?; + harness + .turn_store + .insert(selected_plugins(&[None, Some("env-1:matching")], &[])); + let rendered = harness.render().await?; + assert_eq!(rendered.contains("- deploy:work:"), keeps_remote); + assert!(rendered.contains("- deploy:local:")); + assert!(!rendered.contains("Cloud work description")); + } + Ok(()) +} + +#[tokio::test] +async fn v2_disabled_remote_plugin_blocks_both_read_routes() -> TestResult { + let cloud = cloud_skill("work"); + let installed = + installed_skill("matching", "work")?.with_plugin_id(Some("matching".to_string())); + let harness = MergeHarness::new( + provider(vec![installed.clone()]), + provider(vec![cloud.clone()]), + default_config(), + ) + .await?; + harness.turn_store.insert(selected_plugins( + &[None, Some("env-1:matching")], + &["Plugin_123"], + )); + let tools = harness.tools(&harness.step_store); + assert!(!harness.render().await?.contains("- deploy:work:")); + for package in [&cloud.id, &installed.id] { + let result = call_tool(&tools, "read", json!({"package": package.0})).await; + assert_matches!(result, Err(FunctionCallError::RespondToModel(message)) if message == "skill package is not available"); + } + Ok(()) +} + +#[tokio::test] +async fn v2_rendering_and_tools_retain_their_step_catalog() -> TestResult { + let installed = + installed_skill("matching", "work")?.with_plugin_id(Some("matching".to_string())); + let executor = provider(vec![installed.clone()]); + let executor_calls = executor.list_calls.clone().unwrap(); + let cloud = provider(vec![cloud_skill("work")]); + let cloud_calls = cloud.list_calls.clone().unwrap(); + let harness = MergeHarness::new(executor, cloud, default_config()).await?; + harness + .turn_store + .insert(selected_plugins(&[None, Some("env-1:matching")], &[])); + // Core constructs tools before World State prepares their shared step store. + let original_tools = harness.tools(&harness.step_store); + let early_list = call_tool( + &original_tools, + "list", + json!({"authority": {"kind": "cloud"}}), + ) + .await; + assert_matches!(early_list, Err(FunctionCallError::RespondToModel(message)) if message == "skill catalog is not ready for this step"); + assert_eq!(executor_calls.load(Ordering::SeqCst), 0); + let original_rendering = harness.render().await?; + assert!(original_rendering.contains("- deploy:work:")); + let next_step = ExtensionData::new("step-2"); + let next_tools = harness.tools(&next_step); + harness + .turn_store + .insert(selected_plugins(&[], &["Plugin_123"])); + assert!( + !harness + .render_step(&next_step) + .await? + .contains("- deploy:work:") + ); + // Re-preparing the old step must not replace its catalog with the new policy. + assert_eq!(harness.render().await?, original_rendering); + for (tools, allowed) in [(&original_tools, true), (&next_tools, false)] { + let listed = call_tool(tools, "list", json!({"authority": {"kind": "cloud"}})).await?; + assert_eq!( + listed["skills"].as_array().unwrap().len(), + usize::from(allowed) + ); + let read = call_tool(tools, "read", json!({"package": installed.id.0})).await; + if allowed { + read?; + } else { + assert_matches!(read, Err(FunctionCallError::RespondToModel(message)) if message == "skill package is not available"); + } + } + // Re-enabling the plugin reuses the raw projection, not the denied step's catalog. + let enabled_step = ExtensionData::new("step-3"); + harness + .turn_store + .insert(selected_plugins(&[None, Some("env-1:matching")], &[])); + let enabled_tools = harness.tools(&enabled_step); + assert_eq!( + harness.render_step(&enabled_step).await?, + original_rendering + ); + call_tool(&enabled_tools, "read", json!({"package": installed.id.0})).await?; + assert_eq!(executor_calls.load(Ordering::SeqCst), 1); + assert_eq!(cloud_calls.load(Ordering::SeqCst), 1); + Ok(()) +} + +#[tokio::test] +async fn v2_implicit_and_explicit_invocations_retain_their_step_catalog() -> TestResult { + let installed = + installed_skill("matching", "work")?.with_plugin_id(Some("matching".to_string())); + let cloud = cloud_skill("work").hidden_from_prompt(); + let cloud_provider = provider(vec![cloud.clone()]); + let reads = cloud_provider.read_requests.clone(); + let harness = MergeHarness::new( + provider(vec![installed.clone()]), + cloud_provider, + default_config(), + ) + .await?; + harness + .turn_store + .insert(selected_plugins(&[None, Some("env-1:matching")], &[])); + assert!(!harness.render().await?.contains("- deploy:work:")); + let workdir = PathUri::parse("file:///skills")?; + let detect = |step_store| { + detect_implicit_skill_invocation( + &harness.turn_store, + step_store, + "env-1", + "cat matching/SKILL.md", + &workdir, + /*native_workdir*/ None, + ) + }; + let original = detect(&harness.step_store).expect("prepared step skill"); + assert_eq!(original.remote_plugin_id, cloud.remote_plugin_id); + assert_eq!(original.plugin_id.as_deref(), Some("matching")); + assert_matches!(&original.location, codex_analytics::SkillInvocationLocation::Resource { id, .. } if id == cloud.main_prompt.as_str()); + + let next_step = ExtensionData::new("step-2"); + harness.turn_store.insert(SelectedPluginSnapshot::default()); + harness.render_step(&next_step).await?; + assert_eq!( + detect(&harness.step_store).map(|invocation| ( + invocation.skill_name, + invocation.plugin_id, + invocation.remote_plugin_id, + )), + Some(( + original.skill_name, + original.plugin_id, + original.remote_plugin_id + )), + ); + assert!(detect(&next_step).is_none()); + for (step_store, allowed) in [(&harness.step_store, true), (&next_step, false)] { + let fragments = harness.registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + step_store, + turn_id: "turn-1".to_string(), + user_input: vec![UserInput::Mention { + name: installed.name.clone(), + path: installed.main_prompt.as_str().to_string(), + }], + environments: Vec::new(), + }, + /*extension_metrics*/ None, + &harness.session_store, + &harness.thread_store, + &harness.turn_store, + ) + .await; + assert_eq!(fragments.len(), usize::from(allowed)); + if allowed { + assert!( + fragments[0] + .render() + .expect_text() + .contains("deploy:work") + ); + } + } + assert_eq!( + read_request_keys(&reads), + vec![(cloud.authority, cloud.id, cloud.main_prompt)] + ); + Ok(()) +} + +#[tokio::test] +async fn v1_retains_legacy_plugin_disablement() -> TestResult { + let installed = + installed_skill("legacy", "work")?.with_remote_plugin_id(/*remote_plugin_id*/ None); + for disabled in [false, true] { + let mut executor = provider(vec![installed.clone()]); + executor.catalog.warnings = vec!["Executor warning".into()]; + let reads = executor.read_requests.clone(); + let mut cloud = provider(vec![cloud_skill("work")]); + cloud.catalog.warnings = vec!["Cloud warning".into()]; + let mut harness = MergeHarness::new(executor, cloud, default_config()).await?; + harness.snapshots = None; + harness.discovery = Some(ExecutorCapabilityDiscoverySnapshot::new( + &[], + Vec::new(), + Default::default(), + )); + harness.turn_store.insert(SelectedPluginSnapshot { + plugins: vec![SelectedPluginIdentity { + selected_root_id: Some(installed.authority.id.clone()), + plugin_id: PluginIdentity::Local { + plugin_id: "legacy@marketplace".to_string(), + }, + }], + disabled_plugin_roots: if disabled { + vec![installed.authority.id.clone()] + } else { + Vec::new() + }, + ..Default::default() + }); + let tools = harness.tools(&harness.step_store); + let rendered = harness.render().await?; + assert!(rendered.contains("Cloud work description")); + assert!(!rendered.contains("Installed legacy description")); + let listed = call_tool(&tools, "list", json!({"authority": {"kind": "executor"}})).await?; + assert_eq!( + listed["skills"].as_array().unwrap().len(), + usize::from(!disabled) + ); + assert_eq!(listed["warnings"], json!(["Executor warning"])); + let cloud = call_tool(&tools, "list", json!({"authority": {"kind": "cloud"}})).await?; + assert_eq!(cloud["skills"].as_array().unwrap().len(), 1); + assert_eq!(cloud["warnings"], json!(["Cloud warning"])); + let read = call_tool(&tools, "read", json!({"package": installed.id.0})).await; + if disabled { + assert_matches!(read, Err(FunctionCallError::RespondToModel(message)) if message == "skill package is not available"); + } else { + read?; + } + assert_eq!(read_request_keys(&reads).len(), usize::from(!disabled)); + } + Ok(()) +} + +#[tokio::test] +async fn v2_shadow_observation_tracks_merged_skills_independently_of_read_route() -> TestResult { + for executor_route in [false, true] { + let metrics = MetricsClient::new( + MetricsConfig::in_memory( + "test", + "codex-skills-extension", + env!("CARGO_PKG_VERSION"), + InMemoryMetricExporter::default(), + ) + .with_runtime_reader(), + )?; + let installed = + installed_skill("matching", "work")?.with_plugin_id(Some("matching".to_string())); + let cloud = cloud_skill("work"); + let executor = provider(vec![installed.clone()]); + let cloud_provider = provider(vec![cloud.clone()]); + let executor_reads = executor.read_requests.clone(); + let cloud_reads = cloud_provider.read_requests.clone(); + let mut harness = MergeHarness::with_metrics( + executor, + cloud_provider, + TestConfig { + include_instructions: false, + shadow_selection_enabled: true, + ..default_config() + }, + Some(metrics.clone()), + ) + .await?; + for turn_id in ["turn-1", "turn-2"] { + harness.turn_store = ExtensionData::new(turn_id); + harness.step_store = ExtensionData::new(format!("{turn_id}-step")); + start_registered_turn( + &harness.registry, + &harness.session_store, + &harness.thread_store, + turn_id, + ) + .await; + harness + .turn_store + .insert(selected_plugins(&[None, Some("env-1:matching")], &[])); + let tools = harness.tools(&harness.step_store); + harness.render().await?; + let fragments = harness.registry.turn_input_contributors()[0] + .contribute( + TurnInputContext { + step_store: &harness.step_store, + turn_id: turn_id.to_string(), + user_input: vec![UserInput::Text { + text: "continue".to_string(), + text_elements: Vec::new(), + }], + environments: Vec::new(), + }, + /*extension_metrics*/ None, + &harness.session_store, + &harness.thread_store, + &harness.turn_store, + ) + .await; + assert!(fragments.is_empty()); + if turn_id == "turn-1" { + let entry = if executor_route { &installed } else { &cloud }; + // Repeated reads should yield only one shadow observation for this turn. + for _ in 0..2 { + let output = call_tool(&tools, "read", json!({"package": entry.id.0})).await?; + assert_eq!(output["resource"], json!(entry.main_prompt.as_str())); + } + } else { + // The next turn must recover the observed Cloud candidate in both history selectors. + harness.registry.skill_invocation_contributors()[0] + .on_skill_invocation(SkillInvocationInput { + session_store: &harness.session_store, + thread_store: &harness.thread_store, + turn_store: &harness.turn_store, + turn_id, + skill_resource: cloud.main_prompt.as_str(), + kind: SkillInvocationKind::Implicit, + }) + .await; + } + } + let requested = if executor_route { &installed } else { &cloud }; + let reads = if executor_route { + &executor_reads + } else { + &cloud_reads + }; + let other_reads = if executor_route { + &cloud_reads + } else { + &executor_reads + }; + assert_eq!( + read_request_keys(reads), + vec![ + ( + requested.authority.clone(), + requested.id.clone(), + requested.main_prompt.clone() + ); + if executor_route { 2 } else { 1 } + ] + ); + assert!(read_request_keys(other_reads).is_empty()); + + let snapshots = super::shadow_task_context_tests::collect_shadow_observations( + &metrics, + "codex.skills.shadow_selection.invocation", + /*expected*/ 24, + ) + .await?; + let actual = snapshots + .iter() + .flat_map(opentelemetry_sdk::metrics::data::ResourceMetrics::scope_metrics) + .flat_map(opentelemetry_sdk::metrics::data::ScopeMetrics::metrics) + .filter(|metric| metric.name() == "codex.skills.shadow_selection.invocation") + .flat_map(|metric| match metric.data() { + AggregatedMetrics::U64(MetricData::Sum(sum)) => sum.data_points(), + data => panic!("unexpected shadow metric: {data:?}"), + }) + .filter_map(|point| { + let attribute = |key| { + point + .attributes() + .find(|attribute| attribute.key.as_str() == key) + .map(|attribute| attribute.value.as_str().to_string()) + }; + let method = attribute("method")?; + matches!(method.as_str(), "lru_v1" | "task_context_fusion_v1") + .then(|| ((method, attribute("hit").expect("hit tag")), point.value())) + }) + .fold( + std::collections::BTreeMap::new(), + |mut totals, (key, count)| { + *totals.entry(key).or_insert(0) += count; + totals + }, + ); + let expected = ["lru_v1", "task_context_fusion_v1"] + .into_iter() + .flat_map(|method| { + ["false", "true"] + .into_iter() + .map(move |hit| ((method.to_string(), hit.to_string()), 1)) + }) + .collect::>(); + assert_eq!(actual, expected, "executor_route={executor_route}"); + } + Ok(()) +} diff --git a/codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs b/codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs index c2e011a819..f3c2218a22 100644 --- a/codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs +++ b/codex-rs/ext/skills/tests/skills_extension/presentation_dedup_tests.rs @@ -53,6 +53,7 @@ async fn cloud_preference_preserves_aliases_reads_and_executor_fallback() -> Tes let provider = |entries| { Arc::new(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries, warnings: Vec::new(), }, diff --git a/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs b/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs index a756156aee..5ac944acd4 100644 --- a/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs +++ b/codex-rs/ext/skills/tests/skills_extension/shadow_task_context_tests.rs @@ -88,6 +88,7 @@ async fn task_context_recovers_prior_requests_and_explicit_intent_without_changi opaque.description = "zzzz".to_string(); let provider = Arc::new(FailedReads(StaticSkillProvider { catalog: SkillCatalog { + is_partial: false, entries: vec![ test_entry( SkillSourceKind::Host, @@ -167,6 +168,7 @@ async fn task_context_recovers_prior_requests_and_explicit_intent_without_changi let fragments = registry.turn_input_contributors()[0] .contribute( TurnInputContext { + step_store: &ExtensionData::new("step"), turn_id: turn_id.to_string(), user_input, environments: vec![codex_extension_api::TurnInputEnvironment {