Skip to content

core/app-server: route skill loading through bound path refs #25138

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
starr-openai wants to merge 1 commit into
base: starr/skills-path-ref-3-root-authority
Choose a base branch
from
Draft

core/app-server: route skill loading through bound path refs #25138

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 16 additions & 6 deletions codex-rs/app-server/src/request_processors/catalog_processor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -513,15 +513,19 @@ impl CatalogRequestProcessor {
.await;
let skills_manager = self.thread_manager.skills_manager();
let plugins_manager = self.thread_manager.plugins_manager();
let fs = self
let local_environment = self
.thread_manager
.environment_manager()
.default_environment()
.map(|environment| environment.get_filesystem());
.try_local_environment();
// TODO: Reintroduce env-scoped `skills/list` only after cwd/config-layer discovery can run
// against the selected environment and the design decides which non-repo roots stay local
// versus follow that environment.
let local_file_system = Some(Arc::clone(&codex_exec_server::LOCAL_FS));
let mut data = futures::stream::iter(cwds.into_iter().enumerate())
.map(|(index, cwd)| {
let config = &config;
let fs = fs.clone();
let local_environment = local_environment.clone();
let local_file_system = local_file_system.clone();
let plugins_manager = &plugins_manager;
let skills_manager = &skills_manager;
async move {
Expand Down Expand Up @@ -554,13 +558,19 @@ impl CatalogRequestProcessor {
Vec::new()
};
let skills_input = codex_core::skills::SkillsLoadInput::new(
cwd_abs.clone(),
local_environment.as_ref().map(|environment| {
codex_exec_server::EnvironmentPathRef::new(
environment.get_filesystem(),
cwd_abs.clone(),
)
}),
local_file_system,
effective_skill_roots,
config_layer_stack,
config.bundled_skills_enabled(),
);
let outcome = skills_manager
.skills_for_cwd(&skills_input, force_reload, fs)
.skills_for_cwd(&skills_input, force_reload)
.await;
let errors = errors_to_info(&outcome.errors);
let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths);
Expand Down
14 changes: 9 additions & 5 deletions codex-rs/app-server/src/skills_watcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,14 @@ use codex_core::ThreadManager;
use codex_core::config::Config;
use codex_core::skills::SkillsLoadInput;
use codex_core::skills::SkillsManager;
use codex_exec_server::EnvironmentPathRef;
use codex_file_watcher::FileWatcher;
use codex_file_watcher::FileWatcherSubscriber;
use codex_file_watcher::Receiver;
use codex_file_watcher::ThrottledWatchReceiver;
use codex_file_watcher::WatchPath;
use codex_file_watcher::WatchRegistration;
use codex_protocol::protocol::SkillScope;
use codex_protocol::protocol::TurnEnvironmentSelection;
use codex_utils_absolute_path::AbsolutePathBuf;
use tokio_util::sync::CancellationToken;
Expand Down Expand Up @@ -96,24 +98,26 @@ impl SkillsWatcher {
);
return WatchRegistration::default();
};
if environment.is_remote() {
return WatchRegistration::default();
}

let root_path_ref = (!environment.is_remote())
.then(|| EnvironmentPathRef::new(environment.get_filesystem(), config.cwd.clone()));
let local_file_system = Some(Arc::clone(&codex_exec_server::LOCAL_FS));
let plugins_input = config.plugins_config_input();
let plugins_manager = thread_manager.plugins_manager();
let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await;
let skills_input = SkillsLoadInput::new(
config.cwd.clone(),
root_path_ref,
local_file_system,
plugin_outcome.effective_plugin_skill_roots(),
config.config_layer_stack.clone(),
config.bundled_skills_enabled(),
);
let roots = thread_manager
.skills_manager()
.skill_roots_for_config(&skills_input, Some(environment.get_filesystem()))
.skill_roots_for_config(&skills_input)
.await
.into_iter()
.filter(|root| !environment.is_remote() || root.scope != SkillScope::Repo)
.map(|root| WatchPath {
path: root.path.path().to_path_buf(),
recursive: true,
Expand Down
50 changes: 50 additions & 0 deletions codex-rs/app-server/tests/suite/v2/skills_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,56 @@ async fn skills_list_skips_cwd_roots_when_environment_disabled() -> Result<()> {
Ok(())
}

#[tokio::test]
async fn skills_list_skips_cwd_roots_when_only_remote_environment_is_configured() -> Result<()> {
let codex_home = TempDir::new()?;
let cwd = TempDir::new()?;
write_skill(&codex_home, "home-skill")?;
let repo_skill_dir = cwd.path().join(".codex/skills/repo-skill");
std::fs::create_dir_all(&repo_skill_dir)?;
std::fs::write(
repo_skill_dir.join("SKILL.md"),
"---\nname: repo-skill\ndescription: from repo root\n---\n\n# Body\n",
)?;

let mut mcp = McpProcess::new_with_env(
codex_home.path(),
&[(CODEX_EXEC_SERVER_URL_ENV_VAR, Some("ws://127.0.0.1:8765"))],
)
.await?;
timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??;

let request_id = mcp
.send_skills_list_request(SkillsListParams {
cwds: vec![cwd.path().to_path_buf()],
force_reload: true,
})
.await?;

let response: JSONRPCResponse = timeout(
DEFAULT_TIMEOUT,
mcp.read_stream_until_response_message(RequestId::Integer(request_id)),
)
.await??;
let SkillsListResponse { data } = to_response(response)?;
assert_eq!(data.len(), 1);
assert_eq!(data[0].cwd, cwd.path().to_path_buf());
assert_eq!(data[0].errors, Vec::new());
assert!(
data[0]
.skills
.iter()
.any(|skill| skill.name == "home-skill")
);
assert!(
data[0]
.skills
.iter()
.all(|skill| skill.name != "repo-skill")
);
Ok(())
}

#[tokio::test]
async fn skills_list_accepts_relative_cwds() -> Result<()> {
let codex_home = TempDir::new()?;
Expand Down
71 changes: 21 additions & 50 deletions codex-rs/core-skills/src/manager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,12 +24,12 @@ use crate::system::uninstall_system_skills;
use codex_config::SkillsConfig;
use codex_exec_server::EnvironmentPathRef;
use codex_exec_server::ExecutorFileSystem;
use codex_exec_server::LOCAL_FS;

#[derive(Clone)]
pub struct SkillsLoadInput {
pub cwd: AbsolutePathBuf,
local_file_system: Option<Arc<dyn ExecutorFileSystem>>,
pub env_path: Option<EnvironmentPathRef>,
/// Local user/system/plugin skill roots are read from this filesystem for now.
pub local_file_system: Option<Arc<dyn ExecutorFileSystem>>,
pub effective_skill_roots: Vec<PluginSkillRoot>,
pub config_layer_stack: ConfigLayerStack,
pub bundled_skills_enabled: bool,
Expand All @@ -38,7 +38,7 @@ pub struct SkillsLoadInput {
impl std::fmt::Debug for SkillsLoadInput {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("SkillsLoadInput")
.field("cwd", &self.cwd)
.field("env_path", &self.env_path)
.field("has_local_file_system", &self.local_file_system.is_some())
.field("effective_skill_roots", &self.effective_skill_roots)
.field("config_layer_stack", &self.config_layer_stack)
Expand All @@ -49,41 +49,20 @@ impl std::fmt::Debug for SkillsLoadInput {

impl SkillsLoadInput {
pub fn new(
cwd: AbsolutePathBuf,
env_path: Option<EnvironmentPathRef>,
local_file_system: Option<Arc<dyn ExecutorFileSystem>>,
effective_skill_roots: Vec<PluginSkillRoot>,
config_layer_stack: ConfigLayerStack,
bundled_skills_enabled: bool,
) -> Self {
Self {
cwd,
local_file_system: Some(Arc::clone(&LOCAL_FS)),
env_path,
local_file_system,
effective_skill_roots,
config_layer_stack,
bundled_skills_enabled,
}
}

#[cfg(test)]
fn with_local_file_system(
mut self,
local_file_system: Option<Arc<dyn ExecutorFileSystem>>,
) -> Self {
self.local_file_system = local_file_system;
self
}

fn authorities(
&self,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> (
Option<EnvironmentPathRef>,
Option<Arc<dyn ExecutorFileSystem>>,
) {
(
fs.map(|fs| EnvironmentPathRef::new(fs, self.cwd.clone())),
self.local_file_system.clone(),
)
}
}

pub struct SkillsManager {
Expand Down Expand Up @@ -138,12 +117,8 @@ impl SkillsManager {
/// This path uses a cache keyed by the effective skill-relevant config state rather than just
/// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen
/// to share a directory.
pub async fn skills_for_config(
&self,
input: &SkillsLoadInput,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
let roots = self.skill_roots_for_config(input, fs).await;
pub async fn skills_for_config(&self, input: &SkillsLoadInput) -> SkillLoadOutcome {
let roots = self.skill_roots_for_config(input).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) {
Expand All @@ -159,15 +134,10 @@ impl SkillsManager {
outcome
}

pub async fn skill_roots_for_config(
&self,
input: &SkillsLoadInput,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> Vec<SkillRoot> {
let (env_path, local_file_system) = input.authorities(fs);
pub async fn skill_roots_for_config(&self, input: &SkillsLoadInput) -> Vec<SkillRoot> {
let mut roots = skill_roots(
env_path.as_ref(),
local_file_system.as_ref(),
input.env_path.as_ref(),
input.local_file_system.as_ref(),
&input.config_layer_stack,
input.effective_skill_roots.clone(),
self.extra_roots(),
Expand All @@ -183,12 +153,13 @@ impl SkillsManager {
&self,
input: &SkillsLoadInput,
force_reload: bool,
fs: Option<Arc<dyn ExecutorFileSystem>>,
) -> SkillLoadOutcome {
let (env_path, local_file_system) = input.authorities(fs);
let path_ref_cache_key = env_path.as_ref().map(|_| SkillsPathCacheKey {
env_path: env_path.clone(),
local_file_system: local_file_system.as_ref().map(ExecutorFileSystemRef::new),
let path_ref_cache_key = input.env_path.as_ref().map(|_| SkillsPathCacheKey {
env_path: input.env_path.clone(),
local_file_system: input
.local_file_system
.as_ref()
.map(ExecutorFileSystemRef::new),
});
if !force_reload
&& let Some(path_ref_cache_key) = path_ref_cache_key.as_ref()
Expand All @@ -198,8 +169,8 @@ impl SkillsManager {
}

let mut roots = skill_roots(
env_path.as_ref(),
local_file_system.as_ref(),
input.env_path.as_ref(),
input.local_file_system.as_ref(),
&input.config_layer_stack,
input.effective_skill_roots.clone(),
self.extra_roots(),
Expand Down
Loading
Loading