mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Acquire and cache opt-in executor capability inventories via V2 (#53736)
## What changed - Add the disabled-by-default `executor_capability_discovery_v2` feature to acquire `capabilities/discoverV2` inventories from supporting remote executors using each selected environment's working directory and sandbox. - Share snapshots with MCP and world-state contributors and skill providers without activating discovered plugins or publishing discovered skills to the model. - Cache inventories by environment instance, executor session, working directory, and sandbox. Coalesce concurrent requests, refresh the cache at turn start, retry transient failures, and refresh MCP projections after recovery. - Resolve the system skills root in the executor and pass it explicitly to capability location discovery. ## Testing Add coverage for feature gating, unsupported executors, inert inventories, cache reuse and eviction, turn refreshes, failure recovery, executor replacement, and system skill locations. GitOrigin-RevId: 1bdca28fa7f17622b27e2713ab3dcc0c13426889
This commit is contained in:
committed by
copyberry
parent
4d82d36ccf
commit
cf0ed4c600
@@ -0,0 +1,121 @@
|
||||
//! Bounded lazy V2 responses, cached per environment, executor session, cwd, and sandbox.
|
||||
//! Reports recovery from retryable failures so Core can refresh dependent MCP projections.
|
||||
|
||||
use std::sync::Arc;
|
||||
use std::sync::Weak;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
use std::sync::atomic::AtomicUsize;
|
||||
use std::sync::atomic::Ordering;
|
||||
|
||||
use tokio::sync::Mutex;
|
||||
use tokio::sync::OnceCell;
|
||||
|
||||
use crate::DiscoverV2CapabilitiesRequest;
|
||||
use crate::DiscoverV2CapabilitiesResponse;
|
||||
use crate::Environment;
|
||||
|
||||
const MIN_CACHED_DISCOVERY_CONTEXTS: usize = 16;
|
||||
|
||||
/// Shared by turn and idle consumers; Core replaces its current cache at each turn start.
|
||||
#[derive(Default)]
|
||||
pub struct ExecutorCapabilityDiscoveryV2Cache {
|
||||
entries: Mutex<Vec<Arc<CachedEnvironmentDiscovery>>>,
|
||||
capacity: AtomicUsize,
|
||||
recovered_discovery: AtomicBool,
|
||||
}
|
||||
|
||||
struct CachedEnvironmentDiscovery {
|
||||
request: DiscoverV2CapabilitiesRequest,
|
||||
environment: Weak<Environment>,
|
||||
session_id: Option<String>,
|
||||
response: OnceCell<Result<Arc<DiscoverV2CapabilitiesResponse>, String>>,
|
||||
retryable: Arc<AtomicBool>,
|
||||
}
|
||||
|
||||
impl ExecutorCapabilityDiscoveryV2Cache {
|
||||
/// Fits a complete environment selection; never shrinks until the cache is replaced.
|
||||
pub fn ensure_capacity(&self, minimum: usize) {
|
||||
self.capacity.fetch_max(minimum, Ordering::Relaxed);
|
||||
}
|
||||
|
||||
/// Reports whether a failed discovery recovered since the last observation.
|
||||
pub fn take_recovered_discovery(&self) -> bool {
|
||||
self.recovered_discovery.swap(false, Ordering::AcqRel)
|
||||
}
|
||||
|
||||
/// Reuses successes and non-retryable failures using V1's error classification.
|
||||
/// Retryable failures and cancelled loads leave the cell available for retry.
|
||||
pub async fn discover(
|
||||
&self,
|
||||
environment_id: &str,
|
||||
environment: &Arc<Environment>,
|
||||
request: DiscoverV2CapabilitiesRequest,
|
||||
) -> Result<Arc<DiscoverV2CapabilitiesResponse>, String> {
|
||||
// Resolve startup/replacement before lookup; a resumed session keeps its cache entries.
|
||||
let session_id = environment.executor_session_id().await.map_err(|_| {
|
||||
tracing::warn!(environment_id, "executor discovery V2 session unavailable");
|
||||
"executor discovery V2 failed".to_string()
|
||||
})?;
|
||||
let entry = {
|
||||
let mut entries = self.entries.lock().await;
|
||||
let environment = Arc::downgrade(environment);
|
||||
if let Some(entry) = entries.iter().find(|entry| {
|
||||
entry.environment.ptr_eq(&environment)
|
||||
&& entry.session_id == session_id
|
||||
&& entry.request == request
|
||||
}) {
|
||||
Arc::clone(entry)
|
||||
} else {
|
||||
// Recovery belongs to the environment/request, not one executor session.
|
||||
let retryable = entries
|
||||
.iter()
|
||||
.find(|entry| {
|
||||
entry.environment.ptr_eq(&environment) && entry.request == request
|
||||
})
|
||||
.map(|entry| Arc::clone(&entry.retryable))
|
||||
.unwrap_or_default();
|
||||
let entry = Arc::new(CachedEnvironmentDiscovery {
|
||||
request: request.clone(),
|
||||
environment,
|
||||
session_id,
|
||||
response: OnceCell::new(),
|
||||
retryable,
|
||||
});
|
||||
let capacity = self
|
||||
.capacity
|
||||
.load(Ordering::Relaxed)
|
||||
.max(MIN_CACHED_DISCOVERY_CONTEXTS);
|
||||
if entries.len() == capacity {
|
||||
// Evict the oldest context (FIFO). In-flight requests retain their own Arc.
|
||||
entries.remove(0);
|
||||
}
|
||||
entries.push(Arc::clone(&entry));
|
||||
entry
|
||||
}
|
||||
};
|
||||
let response = entry
|
||||
.response
|
||||
.get_or_try_init(|| async {
|
||||
match environment.discover_capabilities_v2(request).await {
|
||||
Ok(response) => Ok(Ok(Arc::new(response))),
|
||||
Err(error) => {
|
||||
let retryable = crate::client::can_retry_connection_attempt(&error);
|
||||
tracing::warn!(environment_id, "executor discovery V2 failed");
|
||||
let error = "executor discovery V2 failed".to_string();
|
||||
if retryable {
|
||||
entry.retryable.store(true, Ordering::Release);
|
||||
Err(error)
|
||||
} else {
|
||||
Ok(Err(error))
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
.await?
|
||||
.clone();
|
||||
if response.is_ok() && entry.retryable.swap(false, Ordering::AcqRel) {
|
||||
self.recovered_discovery.store(true, Ordering::Release);
|
||||
}
|
||||
response
|
||||
}
|
||||
}
|
||||
@@ -20,6 +20,7 @@ use super::*;
|
||||
use crate::ExecServerRuntimeOptions;
|
||||
use crate::NoiseChannelIdentity;
|
||||
use crate::ProcessId;
|
||||
use crate::discover_v2::capability_locations::tests as location_fixtures;
|
||||
use crate::noise_relay::stream_handler::NoiseOutboundMessage;
|
||||
use crate::noise_relay::stream_handler::NoiseStreamConnection;
|
||||
use crate::noise_relay::stream_handler::NoiseStreamHandler;
|
||||
@@ -285,14 +286,57 @@ async fn refresh_cancels_old_recovery_and_connects_without_resuming_old_session(
|
||||
#[tokio::test]
|
||||
async fn refresh_retires_a_still_connected_old_executor() -> Result<()> {
|
||||
let _clock = freeze_clock();
|
||||
let directory = tempfile::tempdir()?;
|
||||
let root = directory.path().canonicalize()?;
|
||||
for name in ["old", "new"] {
|
||||
location_fixtures::write(
|
||||
&root.join(name),
|
||||
&format!("codex/skills/{name}/SKILL.md"),
|
||||
&format!("---\nname: {name}\ndescription: {name} executor\n---\n"),
|
||||
)?;
|
||||
}
|
||||
let old = Executor::start(Validator::default()).await?;
|
||||
let new = Executor::start(Validator::default()).await?;
|
||||
old.processor
|
||||
.seed_capability_locations(location_fixtures::request(&root.join("old"))?)
|
||||
.await?;
|
||||
new.processor
|
||||
.seed_capability_locations(location_fixtures::request(&root.join("new"))?)
|
||||
.await?;
|
||||
let (client, registry) = old.client()?;
|
||||
let original = client.get().await?;
|
||||
let environment = Arc::new(crate::Environment::remote_with_client(
|
||||
client.clone(),
|
||||
/*local_runtime_paths*/ None,
|
||||
ScopedSkillsConfig::default(),
|
||||
));
|
||||
let cache = crate::ExecutorCapabilityDiscoveryV2Cache::default();
|
||||
let request = crate::DiscoverV2CapabilitiesRequest {
|
||||
cwd: codex_utils_path_uri::PathUri::from_host_native_path(&root)?,
|
||||
sandbox: None,
|
||||
};
|
||||
let inventory = cache
|
||||
.discover("environment", &environment, request.clone())
|
||||
.await
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
assert!(inventory.skills.iter().any(|skill| skill.name == "old"));
|
||||
environment.refresh_connection().await?;
|
||||
let reused = cache
|
||||
.discover("environment", &environment, request.clone())
|
||||
.await
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
assert!(Arc::ptr_eq(&inventory, &reused));
|
||||
*registry.target.lock().unwrap() = new.target.clone();
|
||||
client.refresh_connection().await?;
|
||||
environment.refresh_connection().await?;
|
||||
assert!(original.is_disconnected());
|
||||
assert_ne!(original.session_id(), client.get().await?.session_id());
|
||||
let refreshed = cache
|
||||
.discover("environment", &environment, request)
|
||||
.await
|
||||
.map_err(anyhow::Error::msg)?;
|
||||
assert!(!Arc::ptr_eq(&inventory, &refreshed));
|
||||
assert!(refreshed.skills.iter().any(|skill| skill.name == "new"));
|
||||
assert!(!refreshed.skills.iter().any(|skill| skill.name == "old"));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -375,6 +419,7 @@ async fn registration_renewal_recovers_the_session_and_running_process() -> Resu
|
||||
);
|
||||
let original = client.get().await?;
|
||||
let session_id = original.session_id();
|
||||
assert_eq!(environment.executor_session_id().await?, session_id);
|
||||
let process =
|
||||
original
|
||||
.start_process(
|
||||
@@ -421,6 +466,7 @@ async fn registration_renewal_recovers_the_session_and_running_process() -> Resu
|
||||
let recovered = client.get().await?;
|
||||
assert!(Arc::ptr_eq(&original.inner, &recovered.inner));
|
||||
assert_eq!(recovered.session_id(), session_id);
|
||||
assert_eq!(environment.executor_session_id().await?, session_id);
|
||||
assert_eq!(
|
||||
process.write(b"after-renewal\n".to_vec()).await?.status,
|
||||
crate::protocol::WriteStatus::Accepted
|
||||
|
||||
@@ -8,7 +8,6 @@ use std::io;
|
||||
|
||||
use crate::LocalFileSystem;
|
||||
use crate::protocol::FsOpenMode;
|
||||
use codex_config::loader::system_config_toml_file;
|
||||
use codex_file_system::ExecutorFileSystem;
|
||||
use codex_file_system::WalkEntryKind;
|
||||
use codex_file_system::WalkOptions;
|
||||
@@ -24,11 +23,12 @@ use codex_core_plugin_common::installed::parse_remote_plugin_id;
|
||||
use codex_core_plugin_common::plugin_id::PluginId;
|
||||
use tokio::io::AsyncReadExt;
|
||||
|
||||
/// Home paths are supplied by the executor, not read from the sandbox helper's sanitized env.
|
||||
/// Fixed paths are supplied by the executor, not read from the sandbox helper's sanitized env.
|
||||
#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)]
|
||||
pub(crate) struct CapabilityLocationRequest {
|
||||
pub codex_home: PathUri,
|
||||
pub user_home: Option<PathUri>,
|
||||
pub system_skills_root: Option<PathUri>,
|
||||
}
|
||||
|
||||
const MAX_LOCATIONS: usize = 512;
|
||||
@@ -73,8 +73,8 @@ pub(crate) async fn prewarm_locations(
|
||||
if let Some(home) = user_home {
|
||||
skill_roots.push(home.join(AGENTS_SKILLS_SUFFIX));
|
||||
}
|
||||
if let Some(system_root) = system_config_toml_file()?.parent() {
|
||||
skill_roots.push(system_root.join("skills"));
|
||||
if let Some(system_root) = &request.system_skills_root {
|
||||
skill_roots.push(system_root.to_abs_path()?);
|
||||
}
|
||||
CandidateCapabilityLocations::resolve(
|
||||
fs,
|
||||
@@ -211,4 +211,4 @@ async fn read_remote_plugin_id(path: &std::path::Path) -> io::Result<Option<Stri
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "tests/capability_locations_tests.rs"]
|
||||
pub(super) mod tests;
|
||||
pub(crate) mod tests;
|
||||
|
||||
@@ -9,6 +9,7 @@ use crate::discover_v2::capability_discoveries::load_capability_discoveries;
|
||||
use crate::discover_v2::capability_locations::CapabilityLocationRequest;
|
||||
use crate::discover_v2::capability_locations::CapabilityLocations;
|
||||
use crate::discover_v2::capability_locations::prewarm_locations;
|
||||
use codex_config::loader::system_config_toml_file;
|
||||
use codex_file_watcher::WatchRegistration;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use std::io;
|
||||
@@ -55,7 +56,7 @@ impl CapabilityManager {
|
||||
})
|
||||
}
|
||||
|
||||
/// Resolves home paths on first initialization and reuses the prewarmed locations.
|
||||
/// Resolves fixed paths on first initialization and reuses the prewarmed locations.
|
||||
pub(crate) async fn prewarm_locations(
|
||||
self: &Arc<Self>,
|
||||
) -> io::Result<&(CapabilityLocationRequest, CapabilityLocations)> {
|
||||
@@ -66,6 +67,9 @@ impl CapabilityManager {
|
||||
codex_home: PathUri::from_abs_path(&codex_utils_home_dir::find_codex_home()?),
|
||||
user_home: dirs::home_dir()
|
||||
.and_then(|home| PathUri::from_host_native_path(home).ok()),
|
||||
system_skills_root: system_config_toml_file()?
|
||||
.parent()
|
||||
.map(|root| PathUri::from_abs_path(&root.join("skills"))),
|
||||
};
|
||||
let locations = prewarm_locations(&self.file_system, &request).await?;
|
||||
Ok::<_, io::Error>((request, locations))
|
||||
|
||||
@@ -14,12 +14,15 @@ use crate::LocalFileSystem;
|
||||
pub(crate) const MANIFEST: &str = r#"{"name":"demo","version":"1.10.0"}"#;
|
||||
|
||||
pub(crate) fn request(root: &Path) -> anyhow::Result<CapabilityLocationRequest> {
|
||||
for path in ["codex", "home", "project"] {
|
||||
for path in ["codex", "home", "project", "etc/codex"] {
|
||||
std::fs::create_dir_all(root.join(path))?;
|
||||
}
|
||||
Ok(CapabilityLocationRequest {
|
||||
codex_home: PathUri::from_host_native_path(root.join("codex"))?,
|
||||
user_home: Some(PathUri::from_host_native_path(root.join("home"))?),
|
||||
system_skills_root: Some(PathUri::from_host_native_path(
|
||||
root.join("etc/codex/skills"),
|
||||
)?),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -42,6 +45,7 @@ async fn prewarming_resolves_global_locations_without_cwd_or_config() -> anyhow:
|
||||
for root in [
|
||||
"codex/skills",
|
||||
"home/.agents/skills",
|
||||
"etc/codex/skills",
|
||||
"project/.agents/skills",
|
||||
] {
|
||||
write(temp.path(), &format!("{root}/demo/SKILL.md"), "unparsed")?;
|
||||
@@ -69,7 +73,7 @@ async fn prewarming_resolves_global_locations_without_cwd_or_config() -> anyhow:
|
||||
remote_plugin_id: Some("plugins~demo".to_string()),
|
||||
}),
|
||||
}];
|
||||
for root in ["codex/skills", "home/.agents/skills"] {
|
||||
for root in ["codex/skills", "home/.agents/skills", "etc/codex/skills"] {
|
||||
expected.push(CapabilityLocation {
|
||||
root: PathUri::from_host_native_path(temp.path().join(root))?,
|
||||
plugin: None,
|
||||
|
||||
@@ -6,6 +6,14 @@ use crate::DiscoverV2CapabilitiesResponse;
|
||||
use crate::ExecServerError;
|
||||
|
||||
impl Environment {
|
||||
/// Identifies the initialized executor session; transport recovery preserves this ID.
|
||||
pub(crate) async fn executor_session_id(&self) -> Result<Option<String>, ExecServerError> {
|
||||
match &self.remote_client {
|
||||
Some(client) => Ok(client.get().await?.session_id()),
|
||||
None => Ok(None),
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns metadata for the caller's cwd from the remote session's V2 lifecycle.
|
||||
/// Local environments and older executors retain the V1 selected-root API.
|
||||
pub async fn discover_capabilities_v2(
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
mod arg0_exec_helper;
|
||||
mod capability_discovery;
|
||||
mod capability_discovery_cache;
|
||||
mod capability_discovery_v2_cache;
|
||||
mod client;
|
||||
mod client_api;
|
||||
mod client_error;
|
||||
@@ -71,6 +72,7 @@ pub use arg0_exec_helper::main as run_arg0_exec_helper_main;
|
||||
pub use capability_discovery::CapabilityDiscoveryError;
|
||||
pub use capability_discovery::discover_capability_roots;
|
||||
pub use capability_discovery_cache::ExecutorCapabilityDiscoveryCache;
|
||||
pub use capability_discovery_v2_cache::ExecutorCapabilityDiscoveryV2Cache;
|
||||
pub use client::ExecServerClient;
|
||||
pub use client::ExecServerError;
|
||||
pub use client::http_client::HttpResponseBodyStream;
|
||||
@@ -178,6 +180,7 @@ pub use protocol::ExecResponse;
|
||||
pub use protocol::ExecServerNetworkPolicyDecision;
|
||||
pub use protocol::ExecServerNetworkPolicyRequest;
|
||||
pub use protocol::ExecServerNetworkProtocol;
|
||||
pub use protocol::ExecutorCapabilityDiscoveryV2Snapshot;
|
||||
pub use protocol::ExecutorPlugin;
|
||||
pub use protocol::ExecutorSkill;
|
||||
pub use protocol::FsCanonicalizeParams;
|
||||
|
||||
@@ -327,6 +327,16 @@ mod tests {
|
||||
use crate::server::session_registry::SessionRegistry;
|
||||
|
||||
impl super::ConnectionProcessor {
|
||||
// Discovery tests supply fixture homes without changing the process environment.
|
||||
pub(crate) async fn seed_capability_locations(
|
||||
&self,
|
||||
request: crate::discover_v2::capability_locations::CapabilityLocationRequest,
|
||||
) -> std::io::Result<()> {
|
||||
self.capability_manager
|
||||
.seed_prewarmed_locations(request)
|
||||
.await
|
||||
}
|
||||
|
||||
// Connection-only tests do not prewarm or scan the real user home.
|
||||
pub(crate) fn new(runtime_paths: ExecServerRuntimeOptions) -> Self {
|
||||
let telemetry = crate::ExecServerTelemetry::default();
|
||||
|
||||
Reference in New Issue
Block a user