mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
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
This commit is contained in:
committed by
copyberry
parent
57560e9d53
commit
a309f9206a
Generated
+1
@@ -4693,6 +4693,7 @@ dependencies = [
|
||||
"codex-analytics",
|
||||
"codex-attachment-store",
|
||||
"codex-config",
|
||||
"codex-core-plugin-common",
|
||||
"codex-exec-server",
|
||||
"codex-extension-api",
|
||||
"codex-mcp",
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<String> {
|
||||
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())
|
||||
|
||||
@@ -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<Self>,
|
||||
) -> anyhow::Result<Vec<codex_mcp::ToolInfo>> {
|
||||
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<Arc<ExecutorCapabilityDiscoverySnapshot>> {
|
||||
) -> (Option<Arc<ExecutorCapabilityDiscoverySnapshot>>, 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<Vec<ExecutorCapabilityDiscoveryV2Snapshot>> {
|
||||
) -> (Option<Vec<ExecutorCapabilityDiscoveryV2Snapshot>>, 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(
|
||||
|
||||
@@ -54,3 +54,7 @@ impl Drop for McpRefreshInvalidationGuard<'_> {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "mcp_refresh_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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,
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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::<Vec<_>>();
|
||||
|
||||
let input = TurnInputContext {
|
||||
step_store: &step_context.extension_data,
|
||||
turn_id: turn_context.sub_id.to_string(),
|
||||
user_input: user_input.to_vec(),
|
||||
environments,
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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<Mutex<Vec<ExecutorCapabilityDiscoveryV2Snapshot>>>,
|
||||
selected_plugins: Arc<Mutex<Option<SelectedPluginSnapshot>>>,
|
||||
mcp_snapshots_per_step: Arc<Mutex<Vec<usize>>>,
|
||||
}
|
||||
|
||||
impl McpServerContributor<Config> for DiscoveryRecorder {
|
||||
@@ -48,9 +51,12 @@ impl McpServerContributor<Config> for DiscoveryRecorder {
|
||||
|
||||
fn contribute<'a>(
|
||||
&'a self,
|
||||
_context: McpServerContributionContext<'a, Config>,
|
||||
context: McpServerContributionContext<'a, Config>,
|
||||
) -> ExtensionFuture<'a, Vec<McpServerContribution>> {
|
||||
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<Config> 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<Config> 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<WorldStateSectionContribution>> {
|
||||
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::<SelectedPluginSnapshot>() {
|
||||
*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<Option<Arc<DiscoverV2CapabilitiesResponse>>>,
|
||||
steps: Mutex<
|
||||
Vec<(
|
||||
Arc<DiscoverV2CapabilitiesResponse>,
|
||||
Arc<DiscoverV2CapabilitiesResponse>,
|
||||
)>,
|
||||
>,
|
||||
}
|
||||
|
||||
impl McpServerContributor<Config> for InventoryRecorder {
|
||||
fn id(&self) -> &'static str {
|
||||
"inventory-refresh-recorder"
|
||||
}
|
||||
|
||||
fn contribute<'a>(
|
||||
&'a self,
|
||||
context: McpServerContributionContext<'a, Config>,
|
||||
) -> ExtensionFuture<'a, Vec<McpServerContribution>> {
|
||||
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<WorldStateSectionContribution>> {
|
||||
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::<Vec<_>>();
|
||||
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::<Vec<_>>();
|
||||
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?;
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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(),
|
||||
},
|
||||
|
||||
@@ -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<usize>) -> 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}");
|
||||
|
||||
@@ -41,6 +41,7 @@ impl SkillProvider for CloudTurnProvider {
|
||||
})
|
||||
.collect();
|
||||
Ok(SkillCatalog {
|
||||
is_partial: false,
|
||||
entries,
|
||||
warnings: Vec::new(),
|
||||
})
|
||||
|
||||
@@ -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()),
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -20,6 +20,18 @@ pub struct ExecutorCapabilityDiscoveryV2Snapshot {
|
||||
pub result: Result<Arc<DiscoverV2CapabilitiesResponse>, 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")]
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
//! V2 discovery requests and responses round-trip through JSON.
|
||||
//! V2 discovery wire contracts.
|
||||
|
||||
use super::*;
|
||||
use crate::CapabilityTextFile;
|
||||
|
||||
@@ -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<EnvironmentManager>,
|
||||
entries: Mutex<Vec<CachedRoot>>,
|
||||
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<FileSystemSandboxContext>,
|
||||
environment: Weak<Environment>,
|
||||
session_id: Option<String>,
|
||||
}
|
||||
|
||||
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<Arc<Environment>, String>,
|
||||
}
|
||||
|
||||
struct CachedRoot {
|
||||
key: RootDiscoveryKey,
|
||||
result: Result<Arc<CapabilityRootDiscovery>, String>,
|
||||
// Preserve transport classification after the public snapshot reduces errors to strings.
|
||||
retryable: bool,
|
||||
}
|
||||
|
||||
impl ExecutorCapabilityDiscoveryCache {
|
||||
pub fn new(environment_manager: Arc<EnvironmentManager>) -> 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<String, FileSystemSandboxContext>,
|
||||
) -> Vec<Result<Arc<CapabilityRootDiscovery>, 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::<HashMap<_, _>>();
|
||||
// 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::<HashMap<_, _>>();
|
||||
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::<Vec<_>>();
|
||||
let keys = requests
|
||||
.iter()
|
||||
.map(|request| request.key.clone())
|
||||
.collect::<Vec<_>>();
|
||||
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::<Vec<_>>()
|
||||
};
|
||||
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<String, FileSystemSandboxContext>,
|
||||
) -> 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::<Vec<_>>();
|
||||
let results = self.discover(roots, sandbox_contexts).await;
|
||||
ExecutorCapabilityDiscoverySnapshot::new(&selected_roots, results, sandbox_contexts.clone())
|
||||
}
|
||||
|
||||
async fn discover_missing(
|
||||
&self,
|
||||
missing: Vec<SelectedCapabilityRoot>,
|
||||
sandbox_contexts: &HashMap<String, FileSystemSandboxContext>,
|
||||
) -> Vec<CachedRoot> {
|
||||
let mut grouped = BTreeMap::<String, Vec<SelectedCapabilityRoot>>::new();
|
||||
async fn discover_missing(&self, missing: Vec<RootDiscoveryRequest>) -> Vec<CachedRoot> {
|
||||
let mut grouped = BTreeMap::<String, Vec<RootDiscoveryRequest>>::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::<Vec<_>>()
|
||||
});
|
||||
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::<Vec<_>>();
|
||||
};
|
||||
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()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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(())
|
||||
}
|
||||
|
||||
@@ -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<Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>>;
|
||||
_session_store: &ExtensionData,
|
||||
_thread_store: &ExtensionData,
|
||||
) -> Vec<Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>> {
|
||||
Vec::new()
|
||||
}
|
||||
|
||||
/// Returns native tools bound to one sampling step.
|
||||
fn tools_for_step(
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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,
|
||||
),
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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![
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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 }
|
||||
|
||||
@@ -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<Arc<str>>,
|
||||
}
|
||||
|
||||
/// 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<String>,
|
||||
pub plugin_id: Option<String>,
|
||||
pub analytics_scope: Option<SkillScope>,
|
||||
/// Plugin-service identity for cross-source matching.
|
||||
pub remote_plugin_id: Option<String>,
|
||||
/// Alternate read routes that retain their original authority.
|
||||
pub sources: Vec<SkillSourceLocation>,
|
||||
alias_root: Option<String>,
|
||||
alias_root_order: Option<usize>,
|
||||
prompt_scope: Option<SkillScope>,
|
||||
@@ -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<String>) -> Self {
|
||||
self.remote_plugin_id = remote_plugin_id;
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_plugin_id(mut self, plugin_id: Option<String>) -> Self {
|
||||
self.plugin_id = plugin_id;
|
||||
self
|
||||
}
|
||||
|
||||
pub fn with_short_description(mut self, short_description: Option<String>) -> 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<Item = SkillSourceLocation> + '_ {
|
||||
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<String>,
|
||||
/// Installation attribution belongs to the route used, not the merged primary entry.
|
||||
pub plugin_id: Option<String>,
|
||||
pub enabled: bool,
|
||||
}
|
||||
|
||||
/// Merged catalog for one turn.
|
||||
#[derive(Clone, Debug, Default, PartialEq, Eq)]
|
||||
pub struct SkillCatalog {
|
||||
pub entries: Vec<SkillCatalogEntry>,
|
||||
pub warnings: Vec<String>,
|
||||
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<SkillCatalog>, 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<T> = Result<T, SkillProviderError>;
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "catalog_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -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}",
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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<Option<SkillCatalog>>,
|
||||
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<Arc<McpResourceClient>>) -> 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()
|
||||
},
|
||||
);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -16,10 +16,9 @@ use crate::state::SkillsThreadState;
|
||||
|
||||
impl<C: Send + Sync + 'static> TurnLifecycleContributor for SkillsExtension<C> {
|
||||
fn turn_start_phase(&self, thread_store: &ExtensionData) -> TurnStartPhase {
|
||||
if self.providers.has_cloud_provider()
|
||||
&& thread_store
|
||||
.get::<SkillsThreadState>()
|
||||
.is_some_and(|state| state.cloud_skill_enabled())
|
||||
if thread_store
|
||||
.get::<SkillsThreadState>()
|
||||
.is_some_and(|state| state.cloud_skill_enabled())
|
||||
{
|
||||
TurnStartPhase::RegularTaskStart
|
||||
} else {
|
||||
@@ -36,7 +35,7 @@ impl<C: Send + Sync + 'static> TurnLifecycleContributor for SkillsExtension<C> {
|
||||
let Some(state) = input.thread_store.get::<SkillsThreadState>() 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<C: Send + Sync + 'static> TurnLifecycleContributor for SkillsExtension<C> {
|
||||
.get::<SkillsSessionState>()
|
||||
.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(
|
||||
|
||||
@@ -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"),
|
||||
|
||||
@@ -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::<SkillsThreadState>() {
|
||||
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::<SkillsSessionState>()
|
||||
.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::<SkillsSessionState>()
|
||||
.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<C> ToolContributor for SkillsExtension<C>
|
||||
where
|
||||
C: Send + Sync + 'static,
|
||||
{
|
||||
fn tools(
|
||||
&self,
|
||||
session_store: &ExtensionData,
|
||||
thread_store: &ExtensionData,
|
||||
) -> Vec<Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>> {
|
||||
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<Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>> {
|
||||
let resolved_executor_roots = step_store
|
||||
.get::<Vec<ResolvedSelectedCapabilityRoot>>()
|
||||
.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::<ExecutorCapabilityDiscoverySnapshot>()
|
||||
.map(|discovery| discovery.as_ref().clone()),
|
||||
executor_capability_discovery_v2: step_store
|
||||
.get::<Vec<ExecutorCapabilityDiscoveryV2Snapshot>>()
|
||||
.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::<SelectedPluginSnapshot>(),
|
||||
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::<ExecutorSkillsStepState>()
|
||||
.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::<SkillsStepState>();
|
||||
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::<Vec<ExecutorCapabilityDiscoveryV2Snapshot>>()
|
||||
.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<C> SkillsExtension<C> {
|
||||
fn build_skill_tools(
|
||||
&self,
|
||||
session_store: &ExtensionData,
|
||||
thread_store: &ExtensionData,
|
||||
executor_query: Option<SkillListQuery>,
|
||||
selected_plugins: Option<Arc<SelectedPluginSnapshot>>,
|
||||
) -> Vec<Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>> {
|
||||
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,
|
||||
|
||||
@@ -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::<ExecutorSkillsStepState>()?;
|
||||
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::<crate::skills_extension_state::SkillsStepState>()?;
|
||||
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));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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<SkillInvocation> {
|
||||
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<SkillIn
|
||||
warnings: Vec::new(),
|
||||
};
|
||||
let turn_store = ExtensionData::new("test-turn");
|
||||
turn_store.insert(ExecutorSkillsStepState(catalog));
|
||||
let step_catalog = Arc::new(crate::skills_extension_state::SkillStepCatalog::new(
|
||||
/*cloud*/ None, catalog, /*discovery_v2*/ false,
|
||||
));
|
||||
let step_store = ExtensionData::new("step");
|
||||
step_store.insert(crate::skills_extension_state::SkillsStepState::new_with(
|
||||
Some(step_catalog),
|
||||
));
|
||||
|
||||
detect_implicit_skill_invocation(
|
||||
&turn_store,
|
||||
&step_store,
|
||||
"executor-a",
|
||||
command,
|
||||
&PathUri::parse("file:///skills").expect("valid workdir URI"),
|
||||
@@ -67,3 +78,8 @@ fn ignores_matching_paths_from_other_environments() {
|
||||
assert_eq!(invocation.skill_name, "executor-a-skill");
|
||||
assert!(detect_executor_skill("cat demo/SKILL.md", &["executor-b"]).is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ignores_commands_without_skill_access() {
|
||||
assert!(detect_executor_skill("echo hello", &["executor-a"]).is_none());
|
||||
}
|
||||
|
||||
@@ -28,6 +28,7 @@ mod warnings;
|
||||
mod world_state;
|
||||
mod world_state_catalogs;
|
||||
|
||||
pub use catalog::merge_catalogs;
|
||||
pub use config::SkillsExtensionConfig;
|
||||
pub use extension::install;
|
||||
pub use extension::install_with_providers;
|
||||
|
||||
@@ -39,8 +39,8 @@ pub struct SkillListQuery {
|
||||
pub mcp_resources: Option<Arc<McpResourceClient>>,
|
||||
/// Present only when the opt-in high-level executor discovery path is selected.
|
||||
pub executor_capability_discovery: Option<ExecutorCapabilityDiscoverySnapshot>,
|
||||
/// Inert V2 inventories; empty when none were supplied. Existing discovery is unchanged.
|
||||
pub executor_capability_discovery_v2: Vec<ExecutorCapabilityDiscoveryV2Snapshot>,
|
||||
/// Present when V2 is selected, including empty or failed discovery. Never falls back to V1.
|
||||
pub executor_capability_discovery_v2: Option<Vec<ExecutorCapabilityDiscoveryV2Snapshot>>,
|
||||
}
|
||||
|
||||
/// 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>(
|
||||
|
||||
@@ -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));
|
||||
}
|
||||
|
||||
@@ -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<SkillCatalogEntry> {
|
||||
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,
|
||||
))
|
||||
}
|
||||
}
|
||||
@@ -104,6 +104,7 @@ fn catalog_from_outcome(outcome: &SkillLoadOutcome) -> SkillCatalog {
|
||||
.map(|(index, root)| (root.as_path(), index))
|
||||
.collect::<HashMap<_, _>>();
|
||||
let mut catalog = SkillCatalog {
|
||||
is_partial: false,
|
||||
entries: Vec::new(),
|
||||
warnings: outcome
|
||||
.errors
|
||||
|
||||
@@ -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::<Vec<_>>();
|
||||
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<dyn st
|
||||
include_cloud_skills: false,
|
||||
mcp_resources: None,
|
||||
executor_capability_discovery: None,
|
||||
executor_capability_discovery_v2: Vec::new(),
|
||||
executor_capability_discovery_v2: None,
|
||||
})
|
||||
.await?;
|
||||
let mut entries = catalog.entries.iter().collect::<Vec<_>>();
|
||||
@@ -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::<Vec<_>>();
|
||||
@@ -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(),
|
||||
};
|
||||
|
||||
@@ -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<Item = EnvironmentSkillRequirements<'a>>,
|
||||
) -> Result<(), String> {
|
||||
let mut environments = environments.into_iter().peekable();
|
||||
if environments.peek().is_none() {
|
||||
return Ok(());
|
||||
}
|
||||
let thread_state = thread_store.get::<SkillsThreadState>().ok_or_else(|| {
|
||||
let step_state = step_store.get::<SkillsStepState>().ok_or_else(|| {
|
||||
"Required skills cannot be validated because the skills extension is unavailable"
|
||||
.to_string()
|
||||
})?;
|
||||
let executor = turn_store.get::<ExecutorSkillsStepState>();
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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(),
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<CachedExecutorCatalog>,
|
||||
// Roots-only discovery, reused within a turn.
|
||||
pub(crate) executor_cache: Option<CachedExecutorCatalog>,
|
||||
// DiscoveryV1.
|
||||
pub(crate) executor_discovery_cache: Option<CachedExecutorDiscoveryCatalog>,
|
||||
// DiscoveryV2.
|
||||
pub(crate) executor_v2_cache: Option<CachedExecutorV2Catalog>,
|
||||
// Shared by both discovery modes.
|
||||
pub(crate) cloud_cache: Option<Arc<CloudSkillGeneration>>,
|
||||
}
|
||||
|
||||
/// 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<McpResourceClientAuthKey>,
|
||||
pub(crate) resource_cache_key: Option<McpResourceServerCacheKey>,
|
||||
@@ -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<codex_exec_server::ExecutorCapabilityDiscoveryV2Snapshot>,
|
||||
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<ResolvedSelectedCapabilityRoot>,
|
||||
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<Arc<SkillStepCatalog>>;
|
||||
|
||||
/// 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<Vec<String>>,
|
||||
pub(super) executor_warnings: Vec<String>,
|
||||
}
|
||||
|
||||
impl SkillStepCatalog {
|
||||
pub(crate) fn new(
|
||||
cloud: Option<SkillCatalog>,
|
||||
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
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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,
|
||||
}
|
||||
|
||||
@@ -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<T>(&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<SkillCatalog> {
|
||||
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<CloudSkillGeneration> {
|
||||
@@ -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<SkillCatalogEntry>,
|
||||
pub(crate) warnings: Vec<String>,
|
||||
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);
|
||||
|
||||
@@ -86,7 +86,7 @@ impl<'call> ToolExecutor<ToolCall<'call>> 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
|
||||
|
||||
@@ -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<SkillListQuery>,
|
||||
selected_plugins: Option<Arc<SelectedPluginSnapshot>>,
|
||||
step_store: &ExtensionData,
|
||||
) -> Vec<Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>> {
|
||||
let Some(thread_state) = thread_store.get::<SkillsThreadState>() 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::<Vec<ResolvedSelectedCapabilityRoot>>()
|
||||
.is_some_and(|roots| !roots.is_empty())
|
||||
|| step_store
|
||||
.get::<Vec<ExecutorCapabilityDiscoveryV2Snapshot>>()
|
||||
.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<Arc<McpResourceClient>>,
|
||||
thread_state: Arc<SkillsThreadState>,
|
||||
analytics: Option<SkillAnalytics>,
|
||||
cloud_available: bool,
|
||||
executor_query: Option<SkillListQuery>,
|
||||
selected_plugins: Option<Arc<SelectedPluginSnapshot>>,
|
||||
executor_catalog: Arc<OnceCell<SkillCatalog>>,
|
||||
step_catalog: Arc<SkillsStepState>,
|
||||
}
|
||||
|
||||
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<SkillCatalog, FunctionCallError> {
|
||||
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))
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -87,6 +87,10 @@ impl<'call> ToolExecutor<ToolCall<'call>> 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<ToolCall<'call>> 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::<Vec<_>>();
|
||||
let alias_plan = build_alias_plan(
|
||||
&catalog
|
||||
.entries
|
||||
&entries
|
||||
.iter()
|
||||
.copied()
|
||||
.filter(|entry| entry.is_model_visible())
|
||||
.collect::<Vec<_>>(),
|
||||
);
|
||||
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<ToolCall<'call>> 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<ToolCall<'call>> 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<ToolCall<'call>> for ReadTool {
|
||||
{
|
||||
analytics.track_skill_invocation(
|
||||
&skill_entry,
|
||||
&selected_source,
|
||||
call.model.clone(),
|
||||
call.turn_id.clone(),
|
||||
InvocationType::Implicit,
|
||||
|
||||
@@ -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::<Vec<ResolvedSelectedCapabilityRoot>>();
|
||||
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::<SelectedPluginSnapshot>();
|
||||
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::<SelectedPluginSnapshot>() {
|
||||
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
|
||||
|
||||
@@ -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::<Vec<_>>();
|
||||
|
||||
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()));
|
||||
|
||||
@@ -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<_>>(),
|
||||
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<_>>(),
|
||||
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::<HashMap<_, _>>(),
|
||||
);
|
||||
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::<Vec<_>>()
|
||||
.join("\n");
|
||||
if let Some(expected) = expected {
|
||||
assert!(rendered.contains(expected), "step {index}: {rendered}");
|
||||
} else {
|
||||
assert!(!rendered.contains("- demo:"), "{rendered}");
|
||||
}
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -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<ExecutorCapabilityDiscoveryV2Snapshot, Box<dyn std::error::Error>> {
|
||||
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::<Vec<_>>()
|
||||
.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(())
|
||||
}
|
||||
@@ -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<SkillCatalogEntry, Box<dyn std::error::Error>> {
|
||||
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<SkillCatalogEntry>) -> 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<TestConfig>,
|
||||
session_store: ExtensionData,
|
||||
thread_store: ExtensionData,
|
||||
turn_store: ExtensionData,
|
||||
step_store: ExtensionData,
|
||||
roots: Vec<SelectedCapabilityRoot>,
|
||||
snapshots: Option<Vec<ExecutorCapabilityDiscoveryV2Snapshot>>,
|
||||
discovery: Option<ExecutorCapabilityDiscoverySnapshot>,
|
||||
}
|
||||
|
||||
impl MergeHarness {
|
||||
async fn new(
|
||||
executor: StaticSkillProvider,
|
||||
cloud: StaticSkillProvider,
|
||||
config: TestConfig,
|
||||
) -> Result<Self, Box<dyn std::error::Error>> {
|
||||
Self::with_metrics(executor, cloud, config, /*metrics*/ None).await
|
||||
}
|
||||
|
||||
async fn with_metrics(
|
||||
executor: StaticSkillProvider,
|
||||
cloud: StaticSkillProvider,
|
||||
config: TestConfig,
|
||||
metrics: Option<MetricsClient>,
|
||||
) -> Result<Self, Box<dyn std::error::Error>> {
|
||||
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<Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>> {
|
||||
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<String, Box<dyn std::error::Error>> {
|
||||
self.render_step(&self.step_store).await
|
||||
}
|
||||
|
||||
async fn render_step(
|
||||
&self,
|
||||
step_store: &ExtensionData,
|
||||
) -> Result<String, Box<dyn std::error::Error>> {
|
||||
let turn_store = &self.turn_store;
|
||||
if let Some(plugins) = turn_store.get::<codex_extension_api::SelectedPluginSnapshot>() {
|
||||
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::<Vec<_>>()
|
||||
.join("\n"))
|
||||
}
|
||||
}
|
||||
|
||||
async fn call_tool(
|
||||
tools: &[Arc<dyn for<'call> ToolExecutor<ToolCall<'call>>>],
|
||||
name: &str,
|
||||
arguments: Value,
|
||||
) -> Result<Value, FunctionCallError> {
|
||||
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::<Vec<_>>();
|
||||
assert_eq!(
|
||||
packages,
|
||||
expected_packages
|
||||
.iter()
|
||||
.map(|package| json!(package.0))
|
||||
.collect::<Vec<_>>()
|
||||
);
|
||||
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("<name>deploy:work</name>")
|
||||
);
|
||||
}
|
||||
}
|
||||
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::<std::collections::BTreeMap<_, _>>();
|
||||
assert_eq!(actual, expected, "executor_route={executor_route}");
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
@@ -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(),
|
||||
},
|
||||
|
||||
@@ -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 {
|
||||
|
||||
Reference in New Issue
Block a user