Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion codex-rs/app-server/src/message_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,7 +374,7 @@ impl MessageProcessor {
thread_manager
.plugins_manager()
.set_analytics_events_client(analytics_events_client.clone());
let skills_watcher = SkillsWatcher::new(thread_manager.skills_manager(), outgoing.clone());
let skills_watcher = SkillsWatcher::new(thread_manager.skills_service(), outgoing.clone());

let pending_thread_unloads = Arc::new(Mutex::new(HashSet::new()));
let thread_watch_manager =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ impl AccountRequestProcessor {
) {
tokio::spawn(async move {
thread_manager.plugins_manager().clear_cache();
thread_manager.skills_manager().clear_cache();
thread_manager.skills_service().clear_cache();
if thread_manager.list_thread_ids().await.is_empty() {
return;
}
Expand Down
13 changes: 7 additions & 6 deletions codex-rs/app-server/src/request_processors/catalog_processor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ impl CatalogRequestProcessor {
let workspace_codex_plugins_enabled = self
.workspace_codex_plugins_enabled(&config, auth.as_ref())
.await;
let skills_manager = self.thread_manager.skills_manager();
let skills_service = self.thread_manager.skills_service();
let plugins_manager = self.thread_manager.plugins_manager();
let fs = self
.thread_manager
Expand All @@ -523,7 +523,7 @@ impl CatalogRequestProcessor {
let config = &config;
let fs = fs.clone();
let plugins_manager = &plugins_manager;
let skills_manager = &skills_manager;
let skills_service = &skills_service;
async move {
let (cwd_abs, config_layer_stack) = match self.resolve_cwd_config(&cwd).await {
Ok(resolved) => resolved,
Expand Down Expand Up @@ -559,9 +559,10 @@ impl CatalogRequestProcessor {
config_layer_stack,
config.bundled_skills_enabled(),
);
let outcome = skills_manager
.skills_for_cwd(&skills_input, force_reload, fs)
let snapshot = skills_service
.snapshot_for_cwd(&skills_input, force_reload, fs)
.await;
let outcome = snapshot.outcome();
let errors = errors_to_info(&outcome.errors);
let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths);
(
Expand Down Expand Up @@ -590,7 +591,7 @@ impl CatalogRequestProcessor {
self.skills_watcher
.register_runtime_extra_roots(&extra_roots);
self.thread_manager
.skills_manager()
.skills_service()
.set_extra_roots(extra_roots);
self.outgoing
.send_server_notification(ServerNotification::SkillsChanged(
Expand Down Expand Up @@ -703,7 +704,7 @@ impl CatalogRequestProcessor {
.await
.map(|()| {
self.thread_manager.plugins_manager().clear_cache();
self.thread_manager.skills_manager().clear_cache();
self.thread_manager.skills_service().clear_cache();
SkillsConfigWriteResponse {
effective_enabled: enabled,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ impl ConfigRequestProcessor {

pub(crate) async fn handle_config_mutation(&self) {
self.thread_manager.plugins_manager().clear_cache();
self.thread_manager.skills_manager().clear_cache();
self.thread_manager.skills_service().clear_cache();
}

async fn handle_config_mutation_result<T>(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ impl ExternalAgentConfigRequestProcessor {
completed_item_results.extend(background_item_results);
if has_plugin_imports {
thread_manager.plugins_manager().clear_cache();
thread_manager.skills_manager().clear_cache();
thread_manager.skills_service().clear_cache();
}
send_completed_import_notification(
&outgoing,
Expand Down
4 changes: 2 additions & 2 deletions codex-rs/app-server/src/request_processors/plugins.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ impl PluginRequestProcessor {
) {
tokio::spawn(async move {
thread_manager.plugins_manager().clear_cache();
thread_manager.skills_manager().clear_cache();
thread_manager.skills_service().clear_cache();
if thread_manager.list_thread_ids().await.is_empty() {
return;
}
Expand All @@ -484,7 +484,7 @@ impl PluginRequestProcessor {

fn clear_plugin_related_caches(&self) {
self.thread_manager.plugins_manager().clear_cache();
self.thread_manager.skills_manager().clear_cache();
self.thread_manager.skills_service().clear_cache();
}

async fn load_latest_config(
Expand Down
12 changes: 6 additions & 6 deletions codex-rs/app-server/src/skills_watcher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use codex_app_server_protocol::SkillsChangedNotification;
use codex_core::ThreadManager;
use codex_core::config::Config;
use codex_core::skills::SkillsLoadInput;
use codex_core::skills::SkillsManager;
use codex_core::skills::SkillsService;
use codex_file_watcher::FileWatcher;
use codex_file_watcher::FileWatcherSubscriber;
use codex_file_watcher::Receiver;
Expand All @@ -35,7 +35,7 @@ pub(crate) struct SkillsWatcher {

impl SkillsWatcher {
pub(crate) fn new(
skills_manager: Arc<SkillsManager>,
skills_service: Arc<SkillsService>,
outgoing: Arc<OutgoingMessageSender>,
) -> Arc<Self> {
let file_watcher = match FileWatcher::new() {
Expand All @@ -48,7 +48,7 @@ impl SkillsWatcher {
let (subscriber, rx) = file_watcher.add_subscriber();
let shutdown_token = CancellationToken::new();
let shutdown_drop_guard = shutdown_token.clone().drop_guard();
Self::spawn_event_loop(rx, skills_manager, outgoing, shutdown_token.child_token());
Self::spawn_event_loop(rx, skills_service, outgoing, shutdown_token.child_token());
Arc::new(Self {
subscriber,
runtime_extra_roots_registration: Mutex::new(WatchRegistration::default()),
Expand Down Expand Up @@ -110,7 +110,7 @@ impl SkillsWatcher {
config.bundled_skills_enabled(),
);
let roots = thread_manager
.skills_manager()
.skills_service()
.skill_roots_for_config(&skills_input, Some(environment.get_filesystem()))
.await
.into_iter()
Expand All @@ -124,7 +124,7 @@ impl SkillsWatcher {

fn spawn_event_loop(
rx: Receiver,
skills_manager: Arc<SkillsManager>,
skills_service: Arc<SkillsService>,
outgoing: Arc<OutgoingMessageSender>,
shutdown_token: CancellationToken,
) {
Expand All @@ -142,7 +142,7 @@ impl SkillsWatcher {
if event.is_none() {
break;
}
skills_manager.clear_cache();
skills_service.clear_cache();
outgoing
.send_server_notification(ServerNotification::SkillsChanged(
SkillsChangedNotification {},
Expand Down
2 changes: 1 addition & 1 deletion codex-rs/core-api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ pub use codex_core::config::ThreadStoreConfig;
pub use codex_core::config::find_codex_home;
pub use codex_core::init_state_db;
pub use codex_core::resolve_installation_id;
pub use codex_core::skills::SkillsManager;
pub use codex_core::skills::SkillsService;
Comment thread
jif-oai marked this conversation as resolved.
pub use codex_core::thread_store_from_config;
pub use codex_exec_server::EnvironmentManager;
pub use codex_exec_server::ExecServerError;
Expand Down
8 changes: 4 additions & 4 deletions codex-rs/core-skills/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,18 @@ pub mod config_rules;
pub mod injection;
pub(crate) mod invocation_utils;
pub mod loader;
pub mod manager;
mod mention_counts;
pub mod model;
pub mod remote;
pub mod render;
pub mod service;
mod skill_instructions;
pub mod system;

pub(crate) use invocation_utils::build_implicit_skill_path_indexes;
pub use invocation_utils::detect_implicit_skill_invocation_for_command;
pub use manager::SkillsLoadInput;
pub use manager::SkillsManager;
pub use mention_counts::build_skill_name_counts;
pub use model::HostLoadedSkills;
pub use model::HostSkillsSnapshot;
pub use model::SkillError;
pub use model::SkillLoadOutcome;
pub use model::SkillMetadata;
Expand All @@ -31,4 +29,6 @@ pub use render::SkillRenderReport;
pub use render::build_available_skills;
pub use render::default_skill_metadata_budget;
pub use render::render_available_skills_body;
pub use service::SkillsLoadInput;
pub use service::SkillsService;
pub use skill_instructions::SkillInstructions;
8 changes: 4 additions & 4 deletions codex-rs/core-skills/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -132,14 +132,14 @@ impl SkillLoadOutcome {
}
}

/// Host-loaded skills for one turn, including the filesystem mapping needed to
/// read skill bodies through the environment that loaded them.
/// Immutable snapshot of host-owned skills and the filesystem mapping needed
/// to read each skill through the environment that discovered it.
#[derive(Debug, Clone)]
pub struct HostLoadedSkills {
pub struct HostSkillsSnapshot {
outcome: Arc<SkillLoadOutcome>,
}

impl HostLoadedSkills {
impl HostSkillsSnapshot {
pub fn new(outcome: Arc<SkillLoadOutcome>) -> Self {
Self { outcome }
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use tracing::info;
use tracing::instrument;
use tracing::warn;

use crate::HostSkillsSnapshot;
use crate::SkillLoadOutcome;
use crate::build_implicit_skill_path_indexes;
use crate::config_rules::SkillConfigRules;
Expand Down Expand Up @@ -49,15 +50,18 @@ impl SkillsLoadInput {
}
}

pub struct SkillsManager {
/// Owns host skill discovery, immutable snapshots, cache invalidation, and extra roots.
///
/// Source-specific model exposure remains the responsibility of the skills extension.
pub struct SkillsService {
codex_home: AbsolutePathBuf,
restriction_product: Option<Product>,
extra_roots: RwLock<Vec<AbsolutePathBuf>>,
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, SkillLoadOutcome>>,
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, SkillLoadOutcome>>,
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, HostSkillsSnapshot>>,
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, HostSkillsSnapshot>>,
}

impl SkillsManager {
impl SkillsService {
pub fn new(codex_home: AbsolutePathBuf, bundled_skills_enabled: bool) -> Self {
Self::new_with_restriction_product(codex_home, bundled_skills_enabled, Some(Product::Codex))
}
Expand All @@ -67,7 +71,7 @@ impl SkillsManager {
bundled_skills_enabled: bool,
restriction_product: Option<Product>,
) -> Self {
let manager = Self {
let service = Self {
codex_home,
restriction_product,
extra_roots: RwLock::new(Vec::new()),
Expand All @@ -77,11 +81,11 @@ impl SkillsManager {
if !bundled_skills_enabled {
// The loader caches bundled skills under `skills/.system`. Clearing that directory is
// best-effort cleanup; root selection still enforces the config even if removal fails.
uninstall_system_skills(&manager.codex_home);
} else if let Err(err) = install_system_skills(&manager.codex_home) {
uninstall_system_skills(&service.codex_home);
} else if let Err(err) = install_system_skills(&service.codex_home) {
tracing::error!("failed to install system skills: {err}");
}
manager
service
}

pub fn set_extra_roots(&self, extra_roots: Vec<AbsolutePathBuf>) {
Expand All @@ -102,25 +106,27 @@ impl SkillsManager {
/// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen
/// to share a directory.
#[instrument(level = "trace", skip_all)]
pub async fn skills_for_config(
pub async fn snapshot_for_config(
&self,
input: &SkillsLoadInput,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
) -> HostSkillsSnapshot {
let roots = self.skill_roots_for_config(input, fs).await;
let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack);
let cache_key = config_skills_cache_key(&roots, &skill_config_rules);
if let Some(outcome) = self.cached_outcome_for_config(&cache_key) {
return outcome;
if let Some(snapshot) = self.cached_snapshot_for_config(&cache_key) {
return snapshot;
}

let outcome = self.build_skill_outcome(roots, &skill_config_rules).await;
let snapshot = HostSkillsSnapshot::new(Arc::new(
self.build_skill_outcome(roots, &skill_config_rules).await,
));
let mut cache = self
.cache_by_config
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
cache.insert(cache_key, outcome.clone());
outcome
cache.insert(cache_key, snapshot.clone());
snapshot
}

pub async fn skill_roots_for_config(
Expand All @@ -142,18 +148,18 @@ impl SkillsManager {
roots
}

pub async fn skills_for_cwd(
pub async fn snapshot_for_cwd(
&self,
input: &SkillsLoadInput,
force_reload: bool,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
) -> HostSkillsSnapshot {
let use_cwd_cache = fs.is_some();
if use_cwd_cache
&& !force_reload
&& let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd)
&& let Some(snapshot) = self.cached_snapshot_for_cwd(&input.cwd)
{
return outcome;
return snapshot;
}

let mut roots = skill_roots(
Expand All @@ -168,15 +174,17 @@ impl SkillsManager {
roots.retain(|root| root.scope != SkillScope::System);
}
let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack);
let outcome = self.build_skill_outcome(roots, &skill_config_rules).await;
let snapshot = HostSkillsSnapshot::new(Arc::new(
self.build_skill_outcome(roots, &skill_config_rules).await,
));
if use_cwd_cache {
let mut cache = self
.cache_by_cwd
.write()
.unwrap_or_else(std::sync::PoisonError::into_inner);
cache.insert(input.cwd.clone(), outcome.clone());
cache.insert(input.cwd.clone(), snapshot.clone());
}
outcome
snapshot
}

#[instrument(level = "trace", skip_all)]
Expand Down Expand Up @@ -216,17 +224,17 @@ impl SkillsManager {
info!("skills cache cleared ({cleared} entries)");
}

fn cached_outcome_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option<SkillLoadOutcome> {
fn cached_snapshot_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option<HostSkillsSnapshot> {
match self.cache_by_cwd.read() {
Ok(cache) => cache.get(cwd).cloned(),
Err(err) => err.into_inner().get(cwd).cloned(),
}
}

fn cached_outcome_for_config(
fn cached_snapshot_for_config(
&self,
cache_key: &ConfigSkillsCacheKey,
) -> Option<SkillLoadOutcome> {
) -> Option<HostSkillsSnapshot> {
match self.cache_by_config.read() {
Ok(cache) => cache.get(cache_key).cloned(),
Err(err) => err.into_inner().get(cache_key).cloned(),
Expand Down Expand Up @@ -303,5 +311,5 @@ fn finalize_skill_outcome(
}

#[cfg(test)]
#[path = "manager_tests.rs"]
#[path = "service_tests.rs"]
mod tests;
Loading
Loading