mirror of
https://github.com/openai/codex.git
synced 2026-06-02 19:31:59 +00:00
Compare commits
9 Commits
etraut/thr
...
starr/stag
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
574af01c13 | ||
|
|
41c810bd3d | ||
|
|
beeb747028 | ||
|
|
0899b7c42b | ||
|
|
2d3b3e85ff | ||
|
|
111adfbdb6 | ||
|
|
385010e276 | ||
|
|
7622e229a2 | ||
|
|
5ab860f867 |
@@ -16,13 +16,11 @@ pub(crate) struct CatalogRequestProcessor {
|
||||
const SKILLS_LIST_CWD_CONCURRENCY: usize = 5;
|
||||
|
||||
fn skills_to_info(
|
||||
skills: &[codex_core::skills::SkillMetadata],
|
||||
disabled_paths: &HashSet<AbsolutePathBuf>,
|
||||
outcome: &codex_core::skills::SkillLoadOutcome,
|
||||
) -> Vec<codex_app_server_protocol::SkillMetadata> {
|
||||
skills
|
||||
.iter()
|
||||
.map(|skill| {
|
||||
let enabled = !disabled_paths.contains(&skill.path_to_skills_md);
|
||||
outcome
|
||||
.skills_with_enabled()
|
||||
.map(|(skill, enabled)| {
|
||||
codex_app_server_protocol::SkillMetadata {
|
||||
name: skill.name.clone(),
|
||||
description: skill.description.clone(),
|
||||
@@ -563,7 +561,7 @@ impl CatalogRequestProcessor {
|
||||
.skills_for_cwd(&skills_input, force_reload, fs)
|
||||
.await;
|
||||
let errors = errors_to_info(&outcome.errors);
|
||||
let skills = skills_to_info(&outcome.skills, &outcome.disabled_paths);
|
||||
let skills = skills_to_info(&outcome);
|
||||
(
|
||||
index,
|
||||
codex_app_server_protocol::SkillsListEntry {
|
||||
|
||||
@@ -8,12 +8,33 @@ use codex_file_system::FileSystemSandboxContext;
|
||||
use codex_file_system::ReadDirectoryEntry;
|
||||
use codex_file_system::RemoveOptions;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
use tempfile::tempdir;
|
||||
|
||||
struct TestFileSystem;
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for TestFileSystem {
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
_sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
path.canonicalize()
|
||||
}
|
||||
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
Ok(base_path.join(path))
|
||||
}
|
||||
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult<Option<AbsolutePathBuf>> {
|
||||
Ok(path.parent())
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
@@ -665,7 +665,11 @@ pub async fn load_plugin_skills(
|
||||
.into_iter()
|
||||
.filter(|skill| skill.matches_product_restriction_for_product(restriction_product))
|
||||
.collect::<Vec<_>>();
|
||||
let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules);
|
||||
// Local plugin summaries still expose raw disabled paths outside core-skills.
|
||||
let disabled_skill_paths = resolve_disabled_skill_paths(&skills, skill_config_rules)
|
||||
.into_iter()
|
||||
.map(|path| path.path().clone())
|
||||
.collect();
|
||||
|
||||
ResolvedPluginSkills {
|
||||
skills,
|
||||
|
||||
@@ -5,6 +5,7 @@ use codex_config::ConfigLayerStack;
|
||||
use codex_config::ConfigLayerStackOrdering;
|
||||
use codex_config::SkillConfig;
|
||||
use codex_config::SkillsConfig;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use tracing::warn;
|
||||
|
||||
@@ -71,23 +72,29 @@ pub fn skill_config_rules_from_stack(config_layer_stack: &ConfigLayerStack) -> S
|
||||
pub fn resolve_disabled_skill_paths(
|
||||
skills: &[SkillMetadata],
|
||||
rules: &SkillConfigRules,
|
||||
) -> HashSet<AbsolutePathBuf> {
|
||||
) -> HashSet<EnvironmentPathRef> {
|
||||
let mut disabled_paths = HashSet::new();
|
||||
|
||||
for entry in &rules.entries {
|
||||
match &entry.selector {
|
||||
SkillConfigRuleSelector::Path(path) => {
|
||||
if entry.enabled {
|
||||
disabled_paths.remove(path);
|
||||
} else {
|
||||
disabled_paths.insert(path.clone());
|
||||
for path in skills
|
||||
.iter()
|
||||
.filter(|skill| skill.path_to_skills_md == *path)
|
||||
.map(|skill| skill.source_path.clone())
|
||||
{
|
||||
if entry.enabled {
|
||||
disabled_paths.remove(&path);
|
||||
} else {
|
||||
disabled_paths.insert(path);
|
||||
}
|
||||
}
|
||||
}
|
||||
SkillConfigRuleSelector::Name(name) => {
|
||||
for path in skills
|
||||
.iter()
|
||||
.filter(|skill| skill.name == *name)
|
||||
.map(|skill| skill.path_to_skills_md.clone())
|
||||
.map(|skill| skill.source_path.clone())
|
||||
{
|
||||
if entry.enabled {
|
||||
disabled_paths.remove(&path);
|
||||
|
||||
@@ -1,19 +1,16 @@
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
use crate::SkillLoadOutcome;
|
||||
use crate::SkillMetadata;
|
||||
use crate::build_skill_name_counts;
|
||||
use codex_analytics::AnalyticsEventsClient;
|
||||
use codex_analytics::InvocationType;
|
||||
use codex_analytics::SkillInvocation;
|
||||
use codex_analytics::TrackEventsContext;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_otel::SessionTelemetry;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_plugins::mention_syntax::TOOL_MENTION_SIGIL;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
|
||||
#[derive(Debug, Default)]
|
||||
pub struct SkillInjections {
|
||||
@@ -30,7 +27,6 @@ pub struct SkillInjection {
|
||||
|
||||
pub async fn build_skill_injections(
|
||||
mentioned_skills: &[SkillMetadata],
|
||||
loaded_skills: Option<&SkillLoadOutcome>,
|
||||
otel: Option<&SessionTelemetry>,
|
||||
analytics_client: &AnalyticsEventsClient,
|
||||
tracking: TrackEventsContext,
|
||||
@@ -46,13 +42,7 @@ pub async fn build_skill_injections(
|
||||
let mut invocations = Vec::new();
|
||||
|
||||
for skill in mentioned_skills {
|
||||
let fs = loaded_skills
|
||||
.and_then(|outcome| outcome.file_system_for_skill(skill))
|
||||
.unwrap_or_else(|| Arc::clone(&LOCAL_FS));
|
||||
match fs
|
||||
.read_file_text(&skill.path_to_skills_md, /*sandbox*/ None)
|
||||
.await
|
||||
{
|
||||
match skill.source_path.read_to_string(/*sandbox*/ None).await {
|
||||
Ok(contents) => {
|
||||
emit_skill_injected_metric(otel, skill, "ok");
|
||||
invocations.push(SkillInvocation {
|
||||
@@ -106,7 +96,8 @@ fn emit_skill_injected_metric(
|
||||
/// Structured `UserInput::Skill` selections are resolved first by path against
|
||||
/// enabled skills. Text inputs are then scanned to extract `$skill-name` tokens, and we
|
||||
/// iterate `skills` in their existing order to preserve prior ordering semantics.
|
||||
/// Explicit links are resolved by path and plain names are only used when the match
|
||||
/// Explicit links are resolved by path, using the linked name only when duplicate
|
||||
/// filesystem-bound copies share the same raw path. Plain names are only used when the match
|
||||
/// is unambiguous.
|
||||
///
|
||||
/// Complexity: `O(T + (N_s + N_t) * S)` time, `O(S + M)` space, where:
|
||||
@@ -115,7 +106,7 @@ fn emit_skill_injected_metric(
|
||||
pub fn collect_explicit_skill_mentions(
|
||||
inputs: &[UserInput],
|
||||
skills: &[SkillMetadata],
|
||||
disabled_paths: &HashSet<AbsolutePathBuf>,
|
||||
disabled_paths: &HashSet<EnvironmentPathRef>,
|
||||
connector_slug_counts: &HashMap<String, usize>,
|
||||
) -> Vec<SkillMetadata> {
|
||||
let skill_name_counts = build_skill_name_counts(skills, disabled_paths).0;
|
||||
@@ -128,7 +119,7 @@ pub fn collect_explicit_skill_mentions(
|
||||
};
|
||||
let mut selected: Vec<SkillMetadata> = Vec::new();
|
||||
let mut seen_names: HashSet<String> = HashSet::new();
|
||||
let mut seen_paths: HashSet<AbsolutePathBuf> = HashSet::new();
|
||||
let mut seen_paths: HashSet<EnvironmentPathRef> = HashSet::new();
|
||||
let mut blocked_plain_names: HashSet<String> = HashSet::new();
|
||||
|
||||
for input in inputs {
|
||||
@@ -138,18 +129,23 @@ pub fn collect_explicit_skill_mentions(
|
||||
continue;
|
||||
};
|
||||
|
||||
if selection_context.disabled_paths.contains(&path) || seen_paths.contains(&path) {
|
||||
continue;
|
||||
}
|
||||
|
||||
if let Some(skill) = selection_context
|
||||
let matching_skills = selection_context
|
||||
.skills
|
||||
.iter()
|
||||
.find(|skill| skill.path_to_skills_md == path)
|
||||
.filter(|skill| {
|
||||
skill.name == *name
|
||||
&& skill.path_to_skills_md == path
|
||||
&& !selection_context
|
||||
.disabled_paths
|
||||
.contains(&skill.source_path)
|
||||
})
|
||||
.collect::<Vec<_>>();
|
||||
if let [skill] = matching_skills.as_slice()
|
||||
&& !seen_paths.contains(&skill.source_path)
|
||||
{
|
||||
seen_paths.insert(skill.path_to_skills_md.clone());
|
||||
seen_paths.insert(skill.source_path.clone());
|
||||
seen_names.insert(skill.name.clone());
|
||||
selected.push(skill.clone());
|
||||
selected.push((*skill).clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -173,7 +169,7 @@ pub fn collect_explicit_skill_mentions(
|
||||
|
||||
struct SkillSelectionContext<'a> {
|
||||
skills: &'a [SkillMetadata],
|
||||
disabled_paths: &'a HashSet<AbsolutePathBuf>,
|
||||
disabled_paths: &'a HashSet<EnvironmentPathRef>,
|
||||
skill_name_counts: &'a HashMap<String, usize>,
|
||||
connector_slug_counts: &'a HashMap<String, usize>,
|
||||
}
|
||||
@@ -182,6 +178,7 @@ pub struct ToolMentions<'a> {
|
||||
names: HashSet<&'a str>,
|
||||
paths: HashSet<&'a str>,
|
||||
plain_names: HashSet<&'a str>,
|
||||
linked_mentions: HashSet<LinkedToolMention<'a>>,
|
||||
}
|
||||
|
||||
impl<'a> ToolMentions<'a> {
|
||||
@@ -196,6 +193,16 @@ impl<'a> ToolMentions<'a> {
|
||||
pub fn paths(&self) -> impl Iterator<Item = &'a str> + '_ {
|
||||
self.paths.iter().copied()
|
||||
}
|
||||
|
||||
fn linked_mentions(&self) -> impl Iterator<Item = LinkedToolMention<'a>> + '_ {
|
||||
self.linked_mentions.iter().copied()
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
|
||||
struct LinkedToolMention<'a> {
|
||||
name: &'a str,
|
||||
path: &'a str,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
@@ -260,6 +267,7 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions
|
||||
let mut mentioned_names: HashSet<&str> = HashSet::new();
|
||||
let mut mentioned_paths: HashSet<&str> = HashSet::new();
|
||||
let mut plain_names: HashSet<&str> = HashSet::new();
|
||||
let mut linked_mentions: HashSet<LinkedToolMention<'_>> = HashSet::new();
|
||||
|
||||
let mut index = 0;
|
||||
while index < text_bytes.len() {
|
||||
@@ -276,6 +284,7 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions
|
||||
mentioned_names.insert(name);
|
||||
}
|
||||
mentioned_paths.insert(path);
|
||||
linked_mentions.insert(LinkedToolMention { name, path });
|
||||
}
|
||||
index = end_index;
|
||||
continue;
|
||||
@@ -315,6 +324,7 @@ pub fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions
|
||||
names: mentioned_names,
|
||||
paths: mentioned_paths,
|
||||
plain_names,
|
||||
linked_mentions,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -324,7 +334,7 @@ fn select_skills_from_mentions(
|
||||
blocked_plain_names: &HashSet<String>,
|
||||
mentions: &ToolMentions<'_>,
|
||||
seen_names: &mut HashSet<String>,
|
||||
seen_paths: &mut HashSet<AbsolutePathBuf>,
|
||||
seen_paths: &mut HashSet<EnvironmentPathRef>,
|
||||
selected: &mut Vec<SkillMetadata>,
|
||||
) {
|
||||
if mentions.is_empty() {
|
||||
@@ -341,19 +351,82 @@ fn select_skills_from_mentions(
|
||||
})
|
||||
.map(normalize_skill_path)
|
||||
.collect();
|
||||
let mention_skill_links: HashSet<LinkedToolMention<'_>> = mentions
|
||||
.linked_mentions()
|
||||
.filter(|mention| {
|
||||
!matches!(
|
||||
tool_kind_for_path(mention.path),
|
||||
ToolMentionKind::App | ToolMentionKind::Mcp | ToolMentionKind::Plugin
|
||||
)
|
||||
})
|
||||
.map(|mention| LinkedToolMention {
|
||||
name: mention.name,
|
||||
path: normalize_skill_path(mention.path),
|
||||
})
|
||||
.collect();
|
||||
|
||||
let mention_skill_path_counts = selection_context
|
||||
.skills
|
||||
.iter()
|
||||
.filter(|skill| {
|
||||
!selection_context
|
||||
.disabled_paths
|
||||
.contains(&skill.source_path)
|
||||
})
|
||||
.filter_map(|skill| {
|
||||
let path = skill.path_to_skills_md.to_string_lossy();
|
||||
mention_skill_paths
|
||||
.contains(path.as_ref())
|
||||
.then_some(path.into_owned())
|
||||
})
|
||||
.fold(HashMap::new(), |mut counts, path| {
|
||||
*counts.entry(path).or_insert(0usize) += 1;
|
||||
counts
|
||||
});
|
||||
let mention_skill_link_counts = selection_context
|
||||
.skills
|
||||
.iter()
|
||||
.filter(|skill| {
|
||||
!selection_context
|
||||
.disabled_paths
|
||||
.contains(&skill.source_path)
|
||||
})
|
||||
.filter_map(|skill| {
|
||||
let path = skill.path_to_skills_md.to_string_lossy();
|
||||
mention_skill_links
|
||||
.contains(&LinkedToolMention {
|
||||
name: skill.name.as_str(),
|
||||
path: path.as_ref(),
|
||||
})
|
||||
.then_some((skill.name.clone(), path.into_owned()))
|
||||
})
|
||||
.fold(HashMap::new(), |mut counts, link| {
|
||||
*counts.entry(link).or_insert(0usize) += 1;
|
||||
counts
|
||||
});
|
||||
|
||||
for skill in selection_context.skills {
|
||||
if selection_context
|
||||
.disabled_paths
|
||||
.contains(&skill.path_to_skills_md)
|
||||
|| seen_paths.contains(&skill.path_to_skills_md)
|
||||
.contains(&skill.source_path)
|
||||
|| seen_paths.contains(&skill.source_path)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
|
||||
let path_str = skill.path_to_skills_md.to_string_lossy();
|
||||
if mention_skill_paths.contains(path_str.as_ref()) {
|
||||
seen_paths.insert(skill.path_to_skills_md.clone());
|
||||
let path = path_str.as_ref();
|
||||
let linked_mention = LinkedToolMention {
|
||||
name: skill.name.as_str(),
|
||||
path,
|
||||
};
|
||||
let linked_mention_key = (skill.name.clone(), path.to_string());
|
||||
if mention_skill_paths.contains(path)
|
||||
&& (mention_skill_path_counts.get(path) == Some(&1)
|
||||
|| (mention_skill_links.contains(&linked_mention)
|
||||
&& mention_skill_link_counts.get(&linked_mention_key) == Some(&1)))
|
||||
{
|
||||
seen_paths.insert(skill.source_path.clone());
|
||||
seen_names.insert(skill.name.clone());
|
||||
selected.push(skill.clone());
|
||||
}
|
||||
@@ -362,8 +435,8 @@ fn select_skills_from_mentions(
|
||||
for skill in selection_context.skills {
|
||||
if selection_context
|
||||
.disabled_paths
|
||||
.contains(&skill.path_to_skills_md)
|
||||
|| seen_paths.contains(&skill.path_to_skills_md)
|
||||
.contains(&skill.source_path)
|
||||
|| seen_paths.contains(&skill.source_path)
|
||||
{
|
||||
continue;
|
||||
}
|
||||
@@ -390,7 +463,7 @@ fn select_skills_from_mentions(
|
||||
}
|
||||
|
||||
if seen_names.insert(skill.name.clone()) {
|
||||
seen_paths.insert(skill.path_to_skills_md.clone());
|
||||
seen_paths.insert(skill.source_path.clone());
|
||||
selected.push(skill.clone());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use super::*;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_exec_server::LocalFileSystem;
|
||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||
use codex_utils_absolute_path::test_support::test_path_buf;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -14,6 +15,7 @@ fn make_skill(name: &str, path: &str) -> SkillMetadata {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf(path).abs()),
|
||||
path_to_skills_md: test_path_buf(path).abs(),
|
||||
scope: codex_protocol::protocol::SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -37,7 +39,7 @@ fn linked_skill_mention(name: &str, unix_path: &str) -> String {
|
||||
fn collect_mentions(
|
||||
inputs: &[UserInput],
|
||||
skills: &[SkillMetadata],
|
||||
disabled_paths: &HashSet<AbsolutePathBuf>,
|
||||
disabled_paths: &HashSet<EnvironmentPathRef>,
|
||||
connector_slug_counts: &HashMap<String, usize>,
|
||||
) -> Vec<SkillMetadata> {
|
||||
collect_explicit_skill_mentions(inputs, skills, disabled_paths, connector_slug_counts)
|
||||
@@ -193,6 +195,7 @@ fn collect_explicit_skill_mentions_skips_invalid_structured_and_blocks_plain_fal
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fallback() {
|
||||
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
||||
let disabled = HashSet::from([alpha.source_path.clone()]);
|
||||
let skills = vec![alpha];
|
||||
let inputs = vec![
|
||||
UserInput::Text {
|
||||
@@ -204,7 +207,6 @@ fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fa
|
||||
path: test_path_buf("/tmp/alpha"),
|
||||
},
|
||||
];
|
||||
let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]);
|
||||
let connector_counts = HashMap::new();
|
||||
|
||||
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
|
||||
@@ -212,6 +214,76 @@ fn collect_explicit_skill_mentions_skips_disabled_structured_and_blocks_plain_fa
|
||||
assert_eq!(selected, Vec::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_counts_only_enabled_duplicate_names() {
|
||||
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
||||
let beta = SkillMetadata {
|
||||
source_path: EnvironmentPathRef::new(
|
||||
std::sync::Arc::new(LocalFileSystem::unsandboxed()),
|
||||
test_path_buf("/tmp/beta").abs(),
|
||||
),
|
||||
path_to_skills_md: test_path_buf("/tmp/beta").abs(),
|
||||
..alpha.clone()
|
||||
};
|
||||
let disabled = HashSet::from([beta.source_path.clone()]);
|
||||
let skills = vec![alpha.clone(), beta];
|
||||
let inputs = vec![UserInput::Text {
|
||||
text: "please run $demo-skill".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}];
|
||||
let connector_counts = HashMap::new();
|
||||
|
||||
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
|
||||
|
||||
assert_eq!(selected, vec![alpha]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_uses_structured_name_to_disambiguate_same_path() {
|
||||
let first = make_skill("alpha-skill", "/tmp/shared");
|
||||
let second = SkillMetadata {
|
||||
name: "beta-skill".to_string(),
|
||||
source_path: EnvironmentPathRef::new(
|
||||
std::sync::Arc::new(LocalFileSystem::unsandboxed()),
|
||||
first.path_to_skills_md.clone(),
|
||||
),
|
||||
..first.clone()
|
||||
};
|
||||
let skills = vec![first, second.clone()];
|
||||
let inputs = vec![UserInput::Skill {
|
||||
name: "beta-skill".to_string(),
|
||||
path: test_path_buf("/tmp/shared"),
|
||||
}];
|
||||
let connector_counts = HashMap::new();
|
||||
|
||||
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
||||
|
||||
assert_eq!(selected, vec![second]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_uses_linked_name_to_disambiguate_same_path() {
|
||||
let first = make_skill("alpha-skill", "/tmp/shared");
|
||||
let second = SkillMetadata {
|
||||
name: "beta-skill".to_string(),
|
||||
source_path: EnvironmentPathRef::new(
|
||||
std::sync::Arc::new(LocalFileSystem::unsandboxed()),
|
||||
first.path_to_skills_md.clone(),
|
||||
),
|
||||
..first.clone()
|
||||
};
|
||||
let skills = vec![first, second.clone()];
|
||||
let inputs = vec![UserInput::Text {
|
||||
text: linked_skill_mention("beta-skill", "/tmp/shared"),
|
||||
text_elements: Vec::new(),
|
||||
}];
|
||||
let connector_counts = HashMap::new();
|
||||
|
||||
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
||||
|
||||
assert_eq!(selected, vec![second]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_dedupes_by_path() {
|
||||
let alpha = make_skill("alpha-skill", "/tmp/alpha");
|
||||
@@ -228,6 +300,29 @@ fn collect_explicit_skill_mentions_dedupes_by_path() {
|
||||
assert_eq!(selected, vec![alpha]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_skips_ambiguous_linked_path() {
|
||||
let alpha = make_skill("demo-skill", "/tmp/shared");
|
||||
let beta = SkillMetadata {
|
||||
name: "demo-skill".to_string(),
|
||||
source_path: EnvironmentPathRef::new(
|
||||
std::sync::Arc::new(LocalFileSystem::unsandboxed()),
|
||||
alpha.path_to_skills_md.clone(),
|
||||
),
|
||||
..alpha.clone()
|
||||
};
|
||||
let skills = vec![alpha, beta];
|
||||
let inputs = vec![UserInput::Text {
|
||||
text: linked_skill_mention("demo-skill", "/tmp/shared"),
|
||||
text_elements: Vec::new(),
|
||||
}];
|
||||
let connector_counts = HashMap::new();
|
||||
|
||||
let selected = collect_mentions(&inputs, &skills, &HashSet::new(), &connector_counts);
|
||||
|
||||
assert_eq!(selected, Vec::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_skill_mentions_skips_ambiguous_name() {
|
||||
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
||||
@@ -297,12 +392,12 @@ fn collect_explicit_skill_mentions_allows_explicit_path_with_connector_conflict(
|
||||
fn collect_explicit_skill_mentions_skips_when_linked_path_disabled() {
|
||||
let alpha = make_skill("demo-skill", "/tmp/alpha");
|
||||
let beta = make_skill("demo-skill", "/tmp/beta");
|
||||
let disabled = HashSet::from([alpha.source_path.clone()]);
|
||||
let skills = vec![alpha, beta];
|
||||
let inputs = vec![UserInput::Text {
|
||||
text: format!("use {}", linked_skill_mention("demo-skill", "/tmp/alpha")),
|
||||
text_elements: Vec::new(),
|
||||
}];
|
||||
let disabled = HashSet::from([test_path_buf("/tmp/alpha").abs()]);
|
||||
let connector_counts = HashMap::new();
|
||||
|
||||
let selected = collect_mentions(&inputs, &skills, &disabled, &connector_counts);
|
||||
|
||||
@@ -3,22 +3,25 @@ use std::path::Path;
|
||||
|
||||
use crate::SkillLoadOutcome;
|
||||
use crate::SkillMetadata;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
pub(crate) fn build_implicit_skill_path_indexes(
|
||||
pub(crate) async fn build_implicit_skill_path_indexes(
|
||||
skills: Vec<SkillMetadata>,
|
||||
) -> (
|
||||
HashMap<AbsolutePathBuf, SkillMetadata>,
|
||||
HashMap<AbsolutePathBuf, SkillMetadata>,
|
||||
HashMap<EnvironmentPathRef, SkillMetadata>,
|
||||
HashMap<EnvironmentPathRef, SkillMetadata>,
|
||||
) {
|
||||
let mut by_scripts_dir = HashMap::new();
|
||||
let mut by_skill_doc_path = HashMap::new();
|
||||
for skill in skills {
|
||||
let skill_doc_path = canonicalize_if_exists(&skill.path_to_skills_md);
|
||||
let skill_doc_path = canonicalize_if_exists(&skill.source_path).await;
|
||||
by_skill_doc_path.insert(skill_doc_path, skill.clone());
|
||||
|
||||
if let Some(skill_dir) = skill.path_to_skills_md.parent() {
|
||||
let scripts_dir = canonicalize_if_exists(&skill_dir.join("scripts"));
|
||||
if let Ok(Some(skill_dir)) = skill.source_path.parent().await
|
||||
&& let Ok(scripts_dir) = skill_dir.join(Path::new("scripts")).await
|
||||
{
|
||||
let scripts_dir = canonicalize_if_exists(&scripts_dir).await;
|
||||
by_scripts_dir.insert(scripts_dir, skill);
|
||||
}
|
||||
}
|
||||
@@ -26,19 +29,19 @@ pub(crate) fn build_implicit_skill_path_indexes(
|
||||
(by_scripts_dir, by_skill_doc_path)
|
||||
}
|
||||
|
||||
pub fn detect_implicit_skill_invocation_for_command(
|
||||
pub async fn detect_implicit_skill_invocation_for_command(
|
||||
outcome: &SkillLoadOutcome,
|
||||
command: &str,
|
||||
workdir: &AbsolutePathBuf,
|
||||
workdir: &EnvironmentPathRef,
|
||||
) -> Option<SkillMetadata> {
|
||||
let workdir = canonicalize_if_exists(workdir);
|
||||
let workdir = canonicalize_if_exists(workdir).await;
|
||||
let tokens = tokenize_command(command);
|
||||
|
||||
if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir) {
|
||||
if let Some(candidate) = detect_skill_script_run(outcome, tokens.as_slice(), &workdir).await {
|
||||
return Some(candidate);
|
||||
}
|
||||
|
||||
detect_skill_doc_read(outcome, tokens.as_slice(), &workdir)
|
||||
detect_skill_doc_read(outcome, tokens.as_slice(), &workdir).await
|
||||
}
|
||||
|
||||
fn tokenize_command(command: &str) -> Vec<String> {
|
||||
@@ -78,28 +81,30 @@ fn script_run_token(tokens: &[String]) -> Option<&str> {
|
||||
None
|
||||
}
|
||||
|
||||
fn detect_skill_script_run(
|
||||
async fn detect_skill_script_run(
|
||||
outcome: &SkillLoadOutcome,
|
||||
tokens: &[String],
|
||||
workdir: &AbsolutePathBuf,
|
||||
workdir: &EnvironmentPathRef,
|
||||
) -> Option<SkillMetadata> {
|
||||
let script_token = script_run_token(tokens)?;
|
||||
let script_path = Path::new(script_token);
|
||||
let script_path = canonicalize_if_exists(&workdir.join(script_path));
|
||||
let script_path = canonicalize_if_exists(&resolve_path_ref(workdir, script_path).await?).await;
|
||||
|
||||
for path in script_path.ancestors() {
|
||||
if let Some(candidate) = outcome.implicit_skills_by_scripts_dir.get(&path) {
|
||||
let mut path = Some(script_path);
|
||||
while let Some(candidate_path) = path {
|
||||
if let Some(candidate) = outcome.implicit_skills_by_scripts_dir.get(&candidate_path) {
|
||||
return Some(candidate.clone());
|
||||
}
|
||||
path = candidate_path.parent().await.ok().flatten();
|
||||
}
|
||||
|
||||
None
|
||||
}
|
||||
|
||||
fn detect_skill_doc_read(
|
||||
async fn detect_skill_doc_read(
|
||||
outcome: &SkillLoadOutcome,
|
||||
tokens: &[String],
|
||||
workdir: &AbsolutePathBuf,
|
||||
workdir: &EnvironmentPathRef,
|
||||
) -> Option<SkillMetadata> {
|
||||
if !command_reads_file(tokens) {
|
||||
return None;
|
||||
@@ -110,7 +115,7 @@ fn detect_skill_doc_read(
|
||||
continue;
|
||||
}
|
||||
let path = Path::new(token);
|
||||
let candidate_path = canonicalize_if_exists(&workdir.join(path));
|
||||
let candidate_path = canonicalize_if_exists(&resolve_path_ref(workdir, path).await?).await;
|
||||
if let Some(candidate) = outcome.implicit_skills_by_doc_path.get(&candidate_path) {
|
||||
return Some(candidate.clone());
|
||||
}
|
||||
@@ -136,8 +141,20 @@ fn command_basename(command: &str) -> String {
|
||||
.to_string()
|
||||
}
|
||||
|
||||
fn canonicalize_if_exists(path: &AbsolutePathBuf) -> AbsolutePathBuf {
|
||||
path.canonicalize().unwrap_or_else(|_| path.clone())
|
||||
async fn canonicalize_if_exists(path: &EnvironmentPathRef) -> EnvironmentPathRef {
|
||||
path.canonicalize(/*sandbox*/ None)
|
||||
.await
|
||||
.unwrap_or_else(|_| path.clone())
|
||||
}
|
||||
|
||||
async fn resolve_path_ref(workdir: &EnvironmentPathRef, path: &Path) -> Option<EnvironmentPathRef> {
|
||||
if path.is_absolute() {
|
||||
AbsolutePathBuf::from_absolute_path(path)
|
||||
.ok()
|
||||
.map(|path| workdir.with_path(path))
|
||||
} else {
|
||||
workdir.join(path).await.ok()
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -4,6 +4,7 @@ use super::canonicalize_if_exists;
|
||||
use super::detect_skill_doc_read;
|
||||
use super::detect_skill_script_run;
|
||||
use super::script_run_token;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||
use codex_utils_absolute_path::test_support::test_path_buf;
|
||||
@@ -19,6 +20,7 @@ fn test_skill_metadata(skill_doc_path: AbsolutePathBuf) -> SkillMetadata {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(skill_doc_path.clone()),
|
||||
path_to_skills_md: skill_doc_path,
|
||||
scope: codex_protocol::protocol::SkillScope::User,
|
||||
plugin_id: None,
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
use crate::model::SkillDependencies;
|
||||
use crate::model::SkillError;
|
||||
use crate::model::SkillFileSystemsByPath;
|
||||
use crate::model::SkillInterface;
|
||||
use crate::model::SkillLoadOutcome;
|
||||
use crate::model::SkillMetadata;
|
||||
@@ -13,6 +12,7 @@ use codex_config::ConfigLayerStackOrdering;
|
||||
use codex_config::default_project_root_markers;
|
||||
use codex_config::merge_toml_values;
|
||||
use codex_config::project_root_markers_from_config;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_protocol::protocol::Product;
|
||||
@@ -163,16 +163,15 @@ where
|
||||
I: IntoIterator<Item = SkillRoot>,
|
||||
{
|
||||
let mut outcome = SkillLoadOutcome::default();
|
||||
let mut skill_roots: Vec<AbsolutePathBuf> = Vec::new();
|
||||
let mut skill_root_by_path: HashMap<AbsolutePathBuf, AbsolutePathBuf> = HashMap::new();
|
||||
let mut file_systems_by_skill_path: HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>> =
|
||||
HashMap::new();
|
||||
let mut skill_roots: Vec<EnvironmentPathRef> = Vec::new();
|
||||
let mut skill_root_by_path: HashMap<EnvironmentPathRef, EnvironmentPathRef> = HashMap::new();
|
||||
for root in roots {
|
||||
let root_path = canonicalize_for_skill_identity(&root.path);
|
||||
let fs = root.file_system;
|
||||
let root_source_path = EnvironmentPathRef::new(Arc::clone(&fs), root_path.clone());
|
||||
let skills_before_root = outcome.skills.len();
|
||||
discover_skills_under_root(
|
||||
fs.as_ref(),
|
||||
Arc::clone(&fs),
|
||||
&root_path,
|
||||
root.scope,
|
||||
root.plugin_id.as_deref(),
|
||||
@@ -181,34 +180,29 @@ where
|
||||
)
|
||||
.await;
|
||||
for skill in &outcome.skills[skills_before_root..] {
|
||||
if !skill_roots.contains(&root_path) {
|
||||
skill_roots.push(root_path.clone());
|
||||
if !skill_roots.contains(&root_source_path) {
|
||||
skill_roots.push(root_source_path.clone());
|
||||
}
|
||||
skill_root_by_path
|
||||
.entry(skill.path_to_skills_md.clone())
|
||||
.or_insert_with(|| root_path.clone());
|
||||
file_systems_by_skill_path
|
||||
.entry(skill.path_to_skills_md.clone())
|
||||
.or_insert_with(|| Arc::clone(&fs));
|
||||
.entry(skill.source_path.clone())
|
||||
.or_insert_with(|| root_source_path.clone());
|
||||
}
|
||||
}
|
||||
|
||||
let mut seen: HashSet<AbsolutePathBuf> = HashSet::new();
|
||||
let mut seen: HashSet<EnvironmentPathRef> = HashSet::new();
|
||||
outcome
|
||||
.skills
|
||||
.retain(|skill| seen.insert(skill.path_to_skills_md.clone()));
|
||||
let retained_skill_paths: HashSet<AbsolutePathBuf> = outcome
|
||||
.retain(|skill| seen.insert(skill.source_path.clone()));
|
||||
let retained_skill_paths: HashSet<EnvironmentPathRef> = outcome
|
||||
.skills
|
||||
.iter()
|
||||
.map(|skill| skill.path_to_skills_md.clone())
|
||||
.map(|skill| skill.source_path.clone())
|
||||
.collect();
|
||||
skill_root_by_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
let used_roots: HashSet<AbsolutePathBuf> = skill_root_by_path.values().cloned().collect();
|
||||
let used_roots: HashSet<EnvironmentPathRef> = skill_root_by_path.values().cloned().collect();
|
||||
skill_roots.retain(|root| used_roots.contains(root));
|
||||
file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path));
|
||||
outcome.skill_roots = skill_roots;
|
||||
outcome.skill_root_by_path = Arc::new(skill_root_by_path);
|
||||
outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path);
|
||||
|
||||
fn scope_rank(scope: SkillScope) -> u8 {
|
||||
// Higher-priority scopes first (matches root scan order for dedupe).
|
||||
@@ -466,8 +460,13 @@ fn dirs_between_project_root_and_cwd(
|
||||
}
|
||||
|
||||
fn dedupe_skill_roots_by_path(roots: &mut Vec<SkillRoot>) {
|
||||
let mut seen: HashSet<AbsolutePathBuf> = HashSet::new();
|
||||
roots.retain(|root| seen.insert(root.path.clone()));
|
||||
let mut seen: HashSet<EnvironmentPathRef> = HashSet::new();
|
||||
roots.retain(|root| {
|
||||
seen.insert(EnvironmentPathRef::new(
|
||||
Arc::clone(&root.file_system),
|
||||
root.path.clone(),
|
||||
))
|
||||
});
|
||||
}
|
||||
|
||||
fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf {
|
||||
@@ -475,7 +474,7 @@ fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf {
|
||||
}
|
||||
|
||||
async fn discover_skills_under_root(
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
fs: Arc<dyn ExecutorFileSystem>,
|
||||
root: &AbsolutePathBuf,
|
||||
scope: SkillScope,
|
||||
plugin_id: Option<&str>,
|
||||
@@ -593,7 +592,8 @@ async fn discover_skills_under_root(
|
||||
}
|
||||
|
||||
if metadata.is_file && file_name == SKILLS_FILENAME {
|
||||
match parse_skill_file(fs, &path, scope, plugin_id, plugin_root.as_ref()).await {
|
||||
let source_path = EnvironmentPathRef::new(Arc::clone(&fs), path.clone());
|
||||
match parse_skill_file(source_path, scope, plugin_id, plugin_root.as_ref()).await {
|
||||
Ok(skill) => {
|
||||
outcome.skills.push(skill);
|
||||
}
|
||||
@@ -620,12 +620,13 @@ async fn discover_skills_under_root(
|
||||
}
|
||||
|
||||
async fn parse_skill_file(
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
path: &AbsolutePathBuf,
|
||||
source_path: EnvironmentPathRef,
|
||||
scope: SkillScope,
|
||||
plugin_id: Option<&str>,
|
||||
plugin_root: Option<&AbsolutePathBuf>,
|
||||
) -> Result<SkillMetadata, SkillParseError> {
|
||||
let fs = source_path.file_system();
|
||||
let path = source_path.path();
|
||||
let contents = fs
|
||||
.read_file_text(path, /*sandbox*/ None)
|
||||
.await
|
||||
@@ -642,7 +643,7 @@ async fn parse_skill_file(
|
||||
.map(sanitize_single_line)
|
||||
.filter(|value| !value.is_empty())
|
||||
.unwrap_or_else(|| default_skill_name(path));
|
||||
let name = namespaced_skill_name(fs, path, &base_name).await;
|
||||
let name = namespaced_skill_name(fs.as_ref(), path, &base_name).await;
|
||||
let description = parsed
|
||||
.description
|
||||
.as_deref()
|
||||
@@ -658,7 +659,7 @@ async fn parse_skill_file(
|
||||
interface,
|
||||
dependencies,
|
||||
policy,
|
||||
} = load_skill_metadata(fs, path, plugin_root).await;
|
||||
} = load_skill_metadata(fs.as_ref(), path, plugin_root).await;
|
||||
|
||||
validate_len(&name, MAX_NAME_LEN, "name")?;
|
||||
validate_len(&description, MAX_DESCRIPTION_LEN, "description")?;
|
||||
@@ -679,7 +680,8 @@ async fn parse_skill_file(
|
||||
interface,
|
||||
dependencies,
|
||||
policy,
|
||||
path_to_skills_md: resolved_path,
|
||||
path_to_skills_md: resolved_path.clone(),
|
||||
source_path: source_path.with_path(resolved_path),
|
||||
scope,
|
||||
plugin_id: plugin_id.map(str::to_string),
|
||||
})
|
||||
|
||||
@@ -4,7 +4,9 @@ use codex_config::ConfigLayerEntry;
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_config::ConfigRequirements;
|
||||
use codex_config::ConfigRequirementsToml;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_exec_server::LocalFileSystem;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -330,6 +332,7 @@ async fn loads_skills_from_home_agents_dir_for_user_scope() -> anyhow::Result<()
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -468,6 +471,7 @@ async fn loads_skill_dependencies_metadata_from_yaml() {
|
||||
],
|
||||
}),
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -524,6 +528,7 @@ interface:
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(skill_path.as_path())),
|
||||
path_to_skills_md: normalized(skill_path.as_path()),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -678,6 +683,7 @@ async fn accepts_icon_paths_under_assets_dir() {
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -719,6 +725,7 @@ async fn ignores_invalid_brand_color() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -773,6 +780,7 @@ async fn ignores_default_prompt_over_max_length() {
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -815,6 +823,7 @@ async fn drops_interface_when_icons_are_invalid() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -876,6 +885,7 @@ interface:
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: Some("twilio-developer-kit@test".to_string()),
|
||||
@@ -925,6 +935,7 @@ interface:
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: Some("twilio-developer-kit@test".to_string()),
|
||||
@@ -970,6 +981,7 @@ async fn loads_skills_via_symlinked_subdir_for_user_scope() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&shared_skill_path)),
|
||||
path_to_skills_md: normalized(&shared_skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -1030,6 +1042,7 @@ async fn does_not_loop_on_symlink_cycle_for_user_scope() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -1071,6 +1084,7 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&shared_skill_path)),
|
||||
path_to_skills_md: normalized(&shared_skill_path),
|
||||
scope: SkillScope::Admin,
|
||||
plugin_id: None,
|
||||
@@ -1111,6 +1125,7 @@ async fn loads_skills_via_symlinked_subdir_for_repo_scope() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&linked_skill_path)),
|
||||
path_to_skills_md: normalized(&linked_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1187,6 +1202,7 @@ async fn respects_max_scan_depth_for_user_scope() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&within_depth_path)),
|
||||
path_to_skills_md: normalized(&within_depth_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -1215,6 +1231,7 @@ async fn loads_valid_skill() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -1222,6 +1239,29 @@ async fn loads_valid_skill() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn loaded_skill_source_path_keeps_loader_file_system() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
let skill_path = write_skill(&codex_home, "demo", "demo-skill", "does things");
|
||||
let file_system: Arc<dyn codex_exec_server::ExecutorFileSystem> =
|
||||
Arc::new(LocalFileSystem::unsandboxed());
|
||||
|
||||
let outcome = load_skills_from_roots([SkillRoot {
|
||||
path: skill_path.parent().expect("skill parent"),
|
||||
scope: SkillScope::User,
|
||||
file_system: Arc::clone(&file_system),
|
||||
plugin_id: None,
|
||||
plugin_root: None,
|
||||
}])
|
||||
.await;
|
||||
|
||||
assert!(outcome.errors.is_empty(), "unexpected errors: {:?}", outcome.errors);
|
||||
let skill = outcome.skills.first().expect("loaded skill");
|
||||
assert_eq!(skill.source_path.path(), &skill.path_to_skills_md);
|
||||
assert!(Arc::ptr_eq(&skill.source_path.file_system(), &file_system));
|
||||
assert!(!Arc::ptr_eq(&skill.source_path.file_system(), &LOCAL_FS));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn falls_back_to_directory_name_when_skill_name_is_missing() {
|
||||
let codex_home = tempfile::tempdir().expect("tempdir");
|
||||
@@ -1248,6 +1288,7 @@ async fn falls_back_to_directory_name_when_skill_name_is_missing() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -1294,6 +1335,7 @@ async fn namespaces_plugin_skills_using_plugin_name() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: Some("sample@test".to_string()),
|
||||
@@ -1326,6 +1368,7 @@ async fn loads_short_description_from_metadata() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -1439,6 +1482,7 @@ async fn loads_skills_from_repo_root() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1475,6 +1519,7 @@ async fn loads_skills_from_agents_dir_without_codex_dir() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1529,6 +1574,7 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&nested_skill_path)),
|
||||
path_to_skills_md: normalized(&nested_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1540,6 +1586,7 @@ async fn loads_skills_from_all_codex_dirs_under_project_root() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&root_skill_path)),
|
||||
path_to_skills_md: normalized(&root_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1580,6 +1627,7 @@ async fn loads_skills_from_codex_dir_when_not_git_repo() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1625,6 +1673,7 @@ async fn deduplicates_by_path_preferring_first_root() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1667,6 +1716,7 @@ async fn keeps_duplicate_names_from_repo_and_user() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&repo_skill_path)),
|
||||
path_to_skills_md: normalized(&repo_skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1678,6 +1728,7 @@ async fn keeps_duplicate_names_from_repo_and_user() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&user_skill_path)),
|
||||
path_to_skills_md: normalized(&user_skill_path),
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
@@ -1741,6 +1792,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(first_path.clone()),
|
||||
path_to_skills_md: first_path,
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1752,6 +1804,7 @@ async fn keeps_duplicate_names_from_nested_codex_dirs() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(second_path.clone()),
|
||||
path_to_skills_md: second_path,
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1824,6 +1877,7 @@ async fn loads_skills_when_cwd_is_file_in_repo() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -1883,6 +1937,7 @@ async fn loads_skills_from_system_cache_when_present() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(normalized(&skill_path)),
|
||||
path_to_skills_md: normalized(&skill_path),
|
||||
scope: SkillScope::System,
|
||||
plugin_id: None,
|
||||
|
||||
@@ -4,7 +4,9 @@ use std::sync::Arc;
|
||||
use std::sync::RwLock;
|
||||
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -52,7 +54,7 @@ pub struct SkillsManager {
|
||||
codex_home: AbsolutePathBuf,
|
||||
restriction_product: Option<Product>,
|
||||
extra_roots: RwLock<Vec<AbsolutePathBuf>>,
|
||||
cache_by_cwd: RwLock<HashMap<AbsolutePathBuf, SkillLoadOutcome>>,
|
||||
cache_by_cwd: RwLock<HashMap<EnvironmentPathRef, SkillLoadOutcome>>,
|
||||
cache_by_config: RwLock<HashMap<ConfigSkillsCacheKey, SkillLoadOutcome>>,
|
||||
}
|
||||
|
||||
@@ -147,9 +149,10 @@ impl SkillsManager {
|
||||
fs: Option<Arc<dyn ExecutorFileSystem>>,
|
||||
) -> SkillLoadOutcome {
|
||||
let use_cwd_cache = fs.is_some();
|
||||
let cwd = environment_path_ref(fs.as_ref(), &input.cwd);
|
||||
if use_cwd_cache
|
||||
&& !force_reload
|
||||
&& let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd)
|
||||
&& let Some(outcome) = self.cached_outcome_for_cwd(&cwd)
|
||||
{
|
||||
return outcome;
|
||||
}
|
||||
@@ -172,7 +175,7 @@ impl SkillsManager {
|
||||
.cache_by_cwd
|
||||
.write()
|
||||
.unwrap_or_else(std::sync::PoisonError::into_inner);
|
||||
cache.insert(input.cwd.clone(), outcome.clone());
|
||||
cache.insert(cwd, outcome.clone());
|
||||
}
|
||||
outcome
|
||||
}
|
||||
@@ -187,7 +190,7 @@ impl SkillsManager {
|
||||
self.restriction_product,
|
||||
);
|
||||
let disabled_paths = resolve_disabled_skill_paths(&outcome.skills, skill_config_rules);
|
||||
finalize_skill_outcome(outcome, disabled_paths)
|
||||
finalize_skill_outcome(outcome, disabled_paths).await
|
||||
}
|
||||
|
||||
pub fn clear_cache(&self) {
|
||||
@@ -213,7 +216,7 @@ impl SkillsManager {
|
||||
info!("skills cache cleared ({cleared} entries)");
|
||||
}
|
||||
|
||||
fn cached_outcome_for_cwd(&self, cwd: &AbsolutePathBuf) -> Option<SkillLoadOutcome> {
|
||||
fn cached_outcome_for_cwd(&self, cwd: &EnvironmentPathRef) -> Option<SkillLoadOutcome> {
|
||||
match self.cache_by_cwd.read() {
|
||||
Ok(cache) => cache.get(cwd).cloned(),
|
||||
Err(err) => err.into_inner().get(cwd).cloned(),
|
||||
@@ -240,7 +243,7 @@ impl SkillsManager {
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
|
||||
struct ConfigSkillsCacheKey {
|
||||
roots: Vec<(AbsolutePathBuf, u8, Option<String>)>,
|
||||
roots: Vec<(EnvironmentPathRef, u8, Option<String>)>,
|
||||
skill_config_rules: SkillConfigRules,
|
||||
}
|
||||
|
||||
@@ -280,25 +283,39 @@ fn config_skills_cache_key(
|
||||
SkillScope::System => 2,
|
||||
SkillScope::Admin => 3,
|
||||
};
|
||||
(root.path.clone(), scope_rank, root.plugin_id.clone())
|
||||
(
|
||||
EnvironmentPathRef::new(Arc::clone(&root.file_system), root.path.clone()),
|
||||
scope_rank,
|
||||
root.plugin_id.clone(),
|
||||
)
|
||||
})
|
||||
.collect(),
|
||||
skill_config_rules: skill_config_rules.clone(),
|
||||
}
|
||||
}
|
||||
|
||||
fn finalize_skill_outcome(
|
||||
async fn finalize_skill_outcome(
|
||||
mut outcome: SkillLoadOutcome,
|
||||
disabled_paths: HashSet<AbsolutePathBuf>,
|
||||
disabled_paths: HashSet<EnvironmentPathRef>,
|
||||
) -> SkillLoadOutcome {
|
||||
outcome.disabled_paths = disabled_paths;
|
||||
let (by_scripts_dir, by_doc_path) =
|
||||
build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation());
|
||||
build_implicit_skill_path_indexes(outcome.allowed_skills_for_implicit_invocation()).await;
|
||||
outcome.implicit_skills_by_scripts_dir = Arc::new(by_scripts_dir);
|
||||
outcome.implicit_skills_by_doc_path = Arc::new(by_doc_path);
|
||||
outcome
|
||||
}
|
||||
|
||||
fn environment_path_ref(
|
||||
fs: Option<&Arc<dyn ExecutorFileSystem>>,
|
||||
path: &AbsolutePathBuf,
|
||||
) -> EnvironmentPathRef {
|
||||
EnvironmentPathRef::new(
|
||||
fs.map_or_else(|| Arc::clone(&LOCAL_FS), Arc::clone),
|
||||
path.clone(),
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "manager_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -7,6 +7,7 @@ use codex_config::CONFIG_TOML_FILE;
|
||||
use codex_config::ConfigLayerEntry;
|
||||
use codex_config::ConfigLayerStack;
|
||||
use codex_config::ConfigRequirementsToml;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||
@@ -71,6 +72,11 @@ fn plugin_skill_root_for_skill_path(skill_path: &Path, plugin_id: &str) -> Plugi
|
||||
}
|
||||
|
||||
fn test_skill(name: &str, path: PathBuf) -> SkillMetadata {
|
||||
let path_to_skills_md = path
|
||||
.abs()
|
||||
.canonicalize()
|
||||
.expect("skill path should canonicalize");
|
||||
|
||||
SkillMetadata {
|
||||
name: name.to_string(),
|
||||
description: "test".to_string(),
|
||||
@@ -78,10 +84,8 @@ fn test_skill(name: &str, path: PathBuf) -> SkillMetadata {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
path_to_skills_md: path
|
||||
.abs()
|
||||
.canonicalize()
|
||||
.expect("skill path should canonicalize"),
|
||||
source_path: EnvironmentPathRef::local(path_to_skills_md.clone()),
|
||||
path_to_skills_md,
|
||||
scope: SkillScope::User,
|
||||
plugin_id: None,
|
||||
}
|
||||
@@ -382,7 +386,7 @@ async fn skills_for_config_disables_plugin_skills_by_name() {
|
||||
.abs();
|
||||
|
||||
assert_eq!(skill.path_to_skills_md, skill_path);
|
||||
assert!(outcome.disabled_paths.contains(&skill.path_to_skills_md));
|
||||
assert!(outcome.disabled_paths.contains(&skill.source_path));
|
||||
assert!(
|
||||
!outcome
|
||||
.allowed_skills_for_implicit_invocation()
|
||||
@@ -682,10 +686,12 @@ fn disabled_paths_for_skills_allows_session_flags_to_disable_user_enabled_skill(
|
||||
let skill_config_rules = skill_config_rules_from_stack(&stack);
|
||||
assert_eq!(
|
||||
resolve_disabled_skill_paths(&[skill], &skill_config_rules),
|
||||
HashSet::from([skill_path
|
||||
.abs()
|
||||
.canonicalize()
|
||||
.expect("skill path should canonicalize")])
|
||||
HashSet::from([EnvironmentPathRef::local(
|
||||
skill_path
|
||||
.abs()
|
||||
.canonicalize()
|
||||
.expect("skill path should canonicalize"),
|
||||
)])
|
||||
);
|
||||
}
|
||||
|
||||
@@ -715,10 +721,12 @@ fn disabled_paths_for_skills_disables_matching_name_selectors() {
|
||||
let skill_config_rules = skill_config_rules_from_stack(&stack);
|
||||
assert_eq!(
|
||||
resolve_disabled_skill_paths(&[skill], &skill_config_rules),
|
||||
HashSet::from([skill_path
|
||||
.abs()
|
||||
.canonicalize()
|
||||
.expect("skill path should canonicalize")])
|
||||
HashSet::from([EnvironmentPathRef::local(
|
||||
skill_path
|
||||
.abs()
|
||||
.canonicalize()
|
||||
.expect("skill path should canonicalize"),
|
||||
)])
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -2,17 +2,26 @@ use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
|
||||
use super::SkillMetadata;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
|
||||
/// Counts how often each skill name appears (exact and ASCII-lowercase), excluding disabled paths.
|
||||
pub fn build_skill_name_counts(
|
||||
skills: &[SkillMetadata],
|
||||
disabled_paths: &HashSet<AbsolutePathBuf>,
|
||||
disabled_paths: &HashSet<EnvironmentPathRef>,
|
||||
) -> (HashMap<String, usize>, HashMap<String, usize>) {
|
||||
build_skill_name_counts_with_disabled(skills, |skill| {
|
||||
disabled_paths.contains(&skill.source_path)
|
||||
})
|
||||
}
|
||||
|
||||
fn build_skill_name_counts_with_disabled(
|
||||
skills: &[SkillMetadata],
|
||||
mut is_disabled: impl FnMut(&SkillMetadata) -> bool,
|
||||
) -> (HashMap<String, usize>, HashMap<String, usize>) {
|
||||
let mut exact_counts: HashMap<String, usize> = HashMap::new();
|
||||
let mut lower_counts: HashMap<String, usize> = HashMap::new();
|
||||
for skill in skills {
|
||||
if disabled_paths.contains(&skill.path_to_skills_md) {
|
||||
if is_disabled(skill) {
|
||||
continue;
|
||||
}
|
||||
*exact_counts.entry(skill.name.clone()).or_insert(0) += 1;
|
||||
|
||||
@@ -1,9 +1,8 @@
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::fmt;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -16,8 +15,10 @@ pub struct SkillMetadata {
|
||||
pub interface: Option<SkillInterface>,
|
||||
pub dependencies: Option<SkillDependencies>,
|
||||
pub policy: Option<SkillPolicy>,
|
||||
/// Path to the SKILLS.md file that declares this skill.
|
||||
/// Best-effort resolved raw path used for display, wire payloads, and config selectors.
|
||||
pub path_to_skills_md: AbsolutePathBuf,
|
||||
/// Same resolved path bound to the filesystem that loaded this skill.
|
||||
pub source_path: EnvironmentPathRef,
|
||||
pub scope: SkillScope,
|
||||
pub plugin_id: Option<String>,
|
||||
}
|
||||
@@ -89,17 +90,16 @@ pub struct SkillError {
|
||||
pub struct SkillLoadOutcome {
|
||||
pub skills: Vec<SkillMetadata>,
|
||||
pub errors: Vec<SkillError>,
|
||||
pub disabled_paths: HashSet<AbsolutePathBuf>,
|
||||
pub(crate) skill_roots: Vec<AbsolutePathBuf>,
|
||||
pub(crate) skill_root_by_path: Arc<HashMap<AbsolutePathBuf, AbsolutePathBuf>>,
|
||||
pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath,
|
||||
pub(crate) implicit_skills_by_scripts_dir: Arc<HashMap<AbsolutePathBuf, SkillMetadata>>,
|
||||
pub(crate) implicit_skills_by_doc_path: Arc<HashMap<AbsolutePathBuf, SkillMetadata>>,
|
||||
pub disabled_paths: HashSet<EnvironmentPathRef>,
|
||||
pub(crate) skill_roots: Vec<EnvironmentPathRef>,
|
||||
pub(crate) skill_root_by_path: Arc<HashMap<EnvironmentPathRef, EnvironmentPathRef>>,
|
||||
pub(crate) implicit_skills_by_scripts_dir: Arc<HashMap<EnvironmentPathRef, SkillMetadata>>,
|
||||
pub(crate) implicit_skills_by_doc_path: Arc<HashMap<EnvironmentPathRef, SkillMetadata>>,
|
||||
}
|
||||
|
||||
impl SkillLoadOutcome {
|
||||
pub fn is_skill_enabled(&self, skill: &SkillMetadata) -> bool {
|
||||
!self.disabled_paths.contains(&skill.path_to_skills_md)
|
||||
!self.disabled_paths.contains(&skill.source_path)
|
||||
}
|
||||
|
||||
pub fn is_skill_allowed_for_implicit_invocation(&self, skill: &SkillMetadata) -> bool {
|
||||
@@ -120,47 +120,11 @@ impl SkillLoadOutcome {
|
||||
.map(|skill| (skill, self.is_skill_enabled(skill)))
|
||||
}
|
||||
|
||||
pub(crate) fn file_system_for_skill(
|
||||
&self,
|
||||
skill: &SkillMetadata,
|
||||
) -> Option<Arc<dyn ExecutorFileSystem>> {
|
||||
self.file_systems_by_skill_path
|
||||
.get(&skill.path_to_skills_md)
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Clone, Default)]
|
||||
pub(crate) struct SkillFileSystemsByPath {
|
||||
values: Arc<HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>>>,
|
||||
}
|
||||
|
||||
impl SkillFileSystemsByPath {
|
||||
pub(crate) fn new(values: HashMap<AbsolutePathBuf, Arc<dyn ExecutorFileSystem>>) -> Self {
|
||||
Self {
|
||||
values: Arc::new(values),
|
||||
}
|
||||
}
|
||||
|
||||
fn get(&self, path: &AbsolutePathBuf) -> Option<Arc<dyn ExecutorFileSystem>> {
|
||||
self.values.get(path).map(Arc::clone)
|
||||
}
|
||||
|
||||
fn retain_paths(&mut self, paths: &HashSet<AbsolutePathBuf>) {
|
||||
self.values = Arc::new(
|
||||
self.values
|
||||
.iter()
|
||||
.filter(|(path, _)| paths.contains(*path))
|
||||
.map(|(path, fs)| (path.clone(), Arc::clone(fs)))
|
||||
.collect(),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
impl fmt::Debug for SkillFileSystemsByPath {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
f.debug_struct("SkillFileSystemsByPath")
|
||||
.field("len", &self.values.len())
|
||||
.finish()
|
||||
pub fn disabled_path_values(&self) -> HashSet<AbsolutePathBuf> {
|
||||
self.disabled_paths
|
||||
.iter()
|
||||
.map(|path| path.path().clone())
|
||||
.collect()
|
||||
}
|
||||
}
|
||||
|
||||
@@ -171,14 +135,11 @@ pub fn filter_skill_load_outcome_for_product(
|
||||
outcome
|
||||
.skills
|
||||
.retain(|skill| skill.matches_product_restriction_for_product(restriction_product));
|
||||
let retained_paths: HashSet<AbsolutePathBuf> = outcome
|
||||
let retained_paths: HashSet<EnvironmentPathRef> = outcome
|
||||
.skills
|
||||
.iter()
|
||||
.map(|skill| skill.path_to_skills_md.clone())
|
||||
.map(|skill| skill.source_path.clone())
|
||||
.collect();
|
||||
outcome
|
||||
.file_systems_by_skill_path
|
||||
.retain_paths(&retained_paths);
|
||||
outcome.skill_root_by_path = Arc::new(
|
||||
outcome
|
||||
.skill_root_by_path
|
||||
@@ -187,7 +148,7 @@ pub fn filter_skill_load_outcome_for_product(
|
||||
.map(|(path, root)| (path.clone(), root.clone()))
|
||||
.collect(),
|
||||
);
|
||||
let retained_roots: HashSet<AbsolutePathBuf> =
|
||||
let retained_roots: HashSet<EnvironmentPathRef> =
|
||||
outcome.skill_root_by_path.values().cloned().collect();
|
||||
outcome
|
||||
.skill_roots
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::path::Path;
|
||||
|
||||
use crate::model::SkillLoadOutcome;
|
||||
use crate::model::SkillMetadata;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_otel::SessionTelemetry;
|
||||
use codex_otel::THREAD_SKILLS_DESCRIPTION_TRUNCATED_CHARS_METRIC;
|
||||
use codex_otel::THREAD_SKILLS_ENABLED_TOTAL_METRIC;
|
||||
@@ -665,8 +666,8 @@ struct SkillPathAliases {
|
||||
|
||||
struct AliasPlan {
|
||||
aliases: SkillPathAliases,
|
||||
root_aliases: HashMap<AbsolutePathBuf, String>,
|
||||
alias_root_by_path: HashMap<AbsolutePathBuf, AbsolutePathBuf>,
|
||||
root_aliases: HashMap<EnvironmentPathRef, String>,
|
||||
alias_root_by_path: HashMap<EnvironmentPathRef, EnvironmentPathRef>,
|
||||
table_cost: usize,
|
||||
}
|
||||
|
||||
@@ -677,7 +678,7 @@ fn build_alias_plan(
|
||||
) -> Option<AliasPlan> {
|
||||
let skill_paths = skills
|
||||
.iter()
|
||||
.map(|skill| skill.path_to_skills_md.clone())
|
||||
.map(|skill| skill.source_path.clone())
|
||||
.collect::<HashSet<_>>();
|
||||
let skill_root_by_path = outcome
|
||||
.skill_root_by_path
|
||||
@@ -736,9 +737,9 @@ fn build_alias_plan(
|
||||
}
|
||||
|
||||
fn ordered_alias_roots(
|
||||
used_roots: &[AbsolutePathBuf],
|
||||
alias_root_by_skill_root: &HashMap<AbsolutePathBuf, AbsolutePathBuf>,
|
||||
) -> Option<Vec<AbsolutePathBuf>> {
|
||||
used_roots: &[EnvironmentPathRef],
|
||||
alias_root_by_skill_root: &HashMap<EnvironmentPathRef, EnvironmentPathRef>,
|
||||
) -> Option<Vec<EnvironmentPathRef>> {
|
||||
let mut seen = HashSet::new();
|
||||
let mut alias_roots = Vec::new();
|
||||
for root in used_roots {
|
||||
@@ -751,10 +752,10 @@ fn ordered_alias_roots(
|
||||
}
|
||||
|
||||
fn alias_root_for_skill_root(
|
||||
root: &AbsolutePathBuf,
|
||||
root: &EnvironmentPathRef,
|
||||
plugin_version_skill_counts: &HashMap<AbsolutePathBuf, usize>,
|
||||
) -> AbsolutePathBuf {
|
||||
let Some(plugin_version_base) = plugin_version_base(root.as_path()) else {
|
||||
) -> EnvironmentPathRef {
|
||||
let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) else {
|
||||
return root.clone();
|
||||
};
|
||||
let skill_count = plugin_version_skill_counts
|
||||
@@ -764,16 +765,18 @@ fn alias_root_for_skill_root(
|
||||
if skill_count > 1 {
|
||||
root.clone()
|
||||
} else {
|
||||
plugin_marketplace_base(root.as_path()).unwrap_or_else(|| root.clone())
|
||||
root.with_path(
|
||||
plugin_marketplace_base(root.path().as_path()).unwrap_or_else(|| root.path().clone()),
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
fn plugin_version_skill_counts_for_skill_roots<'a>(
|
||||
skill_roots: impl Iterator<Item = &'a AbsolutePathBuf>,
|
||||
skill_roots: impl Iterator<Item = &'a EnvironmentPathRef>,
|
||||
) -> HashMap<AbsolutePathBuf, usize> {
|
||||
let mut counts = HashMap::new();
|
||||
for root in skill_roots {
|
||||
if let Some(plugin_version_base) = plugin_version_base(root.as_path()) {
|
||||
if let Some(plugin_version_base) = plugin_version_base(root.path().as_path()) {
|
||||
let count = counts.entry(plugin_version_base).or_insert(0usize);
|
||||
*count = count.saturating_add(1);
|
||||
}
|
||||
@@ -793,12 +796,12 @@ fn aliased_metadata_overhead_cost(
|
||||
.saturating_sub(budget.cost(&absolute_body))
|
||||
}
|
||||
|
||||
fn build_skill_root_lines(roots: &[AbsolutePathBuf]) -> Vec<String> {
|
||||
fn build_skill_root_lines(roots: &[EnvironmentPathRef]) -> Vec<String> {
|
||||
roots
|
||||
.iter()
|
||||
.enumerate()
|
||||
.map(|(index, root)| {
|
||||
let root_str = root.to_string_lossy().replace('\\', "/");
|
||||
let root_str = root.path().to_string_lossy().replace('\\', "/");
|
||||
format!("- `r{index}` = `{root_str}`")
|
||||
})
|
||||
.collect()
|
||||
@@ -840,12 +843,13 @@ fn render_skill_path_with_aliases(skill: &SkillMetadata, plan: &AliasPlan) -> St
|
||||
}
|
||||
|
||||
fn outcome_relative_skill_path(skill: &SkillMetadata, plan: &AliasPlan) -> Option<String> {
|
||||
let alias_root = plan.alias_root_by_path.get(&skill.path_to_skills_md)?;
|
||||
let alias_root = plan.alias_root_by_path.get(&skill.source_path)?;
|
||||
let alias = plan.root_aliases.get(alias_root)?;
|
||||
let relative_path = skill
|
||||
.path_to_skills_md
|
||||
.source_path
|
||||
.path()
|
||||
.as_path()
|
||||
.strip_prefix(alias_root.as_path())
|
||||
.strip_prefix(alias_root.path().as_path())
|
||||
.ok()?;
|
||||
let relative_path = relative_path.to_string_lossy().replace('\\', "/");
|
||||
Some(format!("{alias}/{relative_path}"))
|
||||
@@ -905,6 +909,7 @@ fn prompt_scope_rank(scope: SkillScope) -> u8 {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
@@ -920,6 +925,7 @@ mod tests {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs()),
|
||||
path_to_skills_md: test_path_buf(&format!("/tmp/{name}/SKILL.md")).abs(),
|
||||
scope,
|
||||
plugin_id: None,
|
||||
@@ -948,6 +954,10 @@ mod tests {
|
||||
skills: Vec<SkillMetadata>,
|
||||
roots: Vec<AbsolutePathBuf>,
|
||||
) -> SkillLoadOutcome {
|
||||
let roots = roots
|
||||
.into_iter()
|
||||
.map(EnvironmentPathRef::local)
|
||||
.collect::<Vec<_>>();
|
||||
let skill_root_by_path = skills
|
||||
.iter()
|
||||
.filter_map(|skill| {
|
||||
@@ -957,9 +967,9 @@ mod tests {
|
||||
skill
|
||||
.path_to_skills_md
|
||||
.as_path()
|
||||
.starts_with(root.as_path())
|
||||
.starts_with(root.path().as_path())
|
||||
})
|
||||
.map(|root| (skill.path_to_skills_md.clone(), root.clone()))
|
||||
.map(|root| (skill.source_path.clone(), root.clone()))
|
||||
})
|
||||
.collect::<HashMap<_, _>>();
|
||||
SkillLoadOutcome {
|
||||
|
||||
@@ -21,6 +21,7 @@ use codex_config::RequirementSource;
|
||||
use codex_config::Sourced;
|
||||
use codex_config::loader::project_trust_key;
|
||||
use codex_config::types::ToolSuggestDisabledTool;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
|
||||
use codex_features::Feature;
|
||||
use codex_login::CodexAuth;
|
||||
@@ -7265,6 +7266,7 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget()
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/admin-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Admin,
|
||||
plugin_id: None,
|
||||
@@ -7276,6 +7278,7 @@ async fn build_initial_context_trims_skill_metadata_from_context_window_budget()
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/repo-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -7312,6 +7315,7 @@ fn emit_thread_start_skill_metrics_records_enabled_kept_and_truncated_values() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/repo-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -7357,6 +7361,7 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/alpha-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/alpha-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -7368,6 +7373,7 @@ fn emit_thread_start_skill_metrics_records_description_truncated_chars_without_o
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/beta-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/beta-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
@@ -7416,6 +7422,7 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/admin-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/admin-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Admin,
|
||||
plugin_id: None,
|
||||
@@ -7427,6 +7434,7 @@ async fn build_initial_context_emits_thread_start_skill_warning_on_repeated_buil
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/repo-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/repo-skill/SKILL.md").abs(),
|
||||
scope: SkillScope::Repo,
|
||||
plugin_id: None,
|
||||
|
||||
@@ -533,7 +533,6 @@ async fn build_skills_and_plugins(
|
||||
warnings: skill_warnings,
|
||||
} = build_skill_injections(
|
||||
&mentioned_skills,
|
||||
Some(skills_outcome),
|
||||
Some(&turn_context.session_telemetry),
|
||||
&sess.services.analytics_events_client,
|
||||
tracking.clone(),
|
||||
|
||||
@@ -2,6 +2,7 @@ use super::*;
|
||||
use crate::SkillLoadOutcome;
|
||||
use crate::config::GhostSnapshotConfig;
|
||||
use crate::environment_selection::ResolvedTurnEnvironments;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_model_provider::SharedModelProvider;
|
||||
use codex_model_provider::create_model_provider;
|
||||
use codex_protocol::SessionId;
|
||||
@@ -19,7 +20,7 @@ use std::sync::atomic::Ordering;
|
||||
#[derive(Clone, Debug)]
|
||||
pub(crate) struct TurnSkillsContext {
|
||||
pub(crate) outcome: Arc<SkillLoadOutcome>,
|
||||
pub(crate) implicit_invocation_seen_skills: Arc<Mutex<HashSet<String>>>,
|
||||
pub(crate) implicit_invocation_seen_skills: Arc<Mutex<HashSet<EnvironmentPathRef>>>,
|
||||
}
|
||||
|
||||
impl TurnSkillsContext {
|
||||
|
||||
@@ -4,8 +4,7 @@ use crate::session::turn_context::TurnContext;
|
||||
use codex_analytics::InvocationType;
|
||||
use codex_analytics::SkillInvocation;
|
||||
use codex_analytics::build_track_events_context;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_utils_plugins::PluginSkillRoot;
|
||||
|
||||
pub use codex_core_skills::SkillError;
|
||||
@@ -49,15 +48,18 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
|
||||
sess: &Session,
|
||||
turn_context: &TurnContext,
|
||||
command: &str,
|
||||
workdir: &AbsolutePathBuf,
|
||||
workdir: &EnvironmentPathRef,
|
||||
) {
|
||||
let Some(candidate) = detect_implicit_skill_invocation_for_command(
|
||||
turn_context.turn_skills.outcome.as_ref(),
|
||||
command,
|
||||
workdir,
|
||||
) else {
|
||||
)
|
||||
.await
|
||||
else {
|
||||
return;
|
||||
};
|
||||
let source_path = candidate.source_path.clone();
|
||||
let invocation = SkillInvocation {
|
||||
skill_name: candidate.name,
|
||||
skill_scope: candidate.scope,
|
||||
@@ -65,22 +67,14 @@ pub(crate) async fn maybe_emit_implicit_skill_invocation(
|
||||
plugin_id: candidate.plugin_id,
|
||||
invocation_type: InvocationType::Implicit,
|
||||
};
|
||||
let skill_scope = match invocation.skill_scope {
|
||||
SkillScope::User => "user",
|
||||
SkillScope::Repo => "repo",
|
||||
SkillScope::System => "system",
|
||||
SkillScope::Admin => "admin",
|
||||
};
|
||||
let skill_path = invocation.skill_path.to_string_lossy();
|
||||
let skill_name = invocation.skill_name.clone();
|
||||
let seen_key = format!("{skill_scope}:{skill_path}:{skill_name}");
|
||||
let inserted = {
|
||||
let mut seen_skills = turn_context
|
||||
.turn_skills
|
||||
.implicit_invocation_seen_skills
|
||||
.lock()
|
||||
.await;
|
||||
seen_skills.insert(seen_key)
|
||||
seen_skills.insert(source_path)
|
||||
};
|
||||
if !inserted {
|
||||
return;
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::models::ShellCommandToolCallParams;
|
||||
use codex_tools::ShellCommandBackendConfig;
|
||||
@@ -167,6 +168,11 @@ impl ToolExecutor<ToolInvocation> for ShellCommandHandler {
|
||||
let params: ShellCommandToolCallParams = parse_arguments_with_base_path(&arguments, &cwd)?;
|
||||
#[allow(deprecated)]
|
||||
let workdir = turn.resolve_path(params.workdir.clone());
|
||||
let workdir = turn
|
||||
.environments
|
||||
.primary_filesystem()
|
||||
.map(|file_system| EnvironmentPathRef::new(file_system, workdir.clone()))
|
||||
.unwrap_or_else(|| EnvironmentPathRef::local(workdir.clone()));
|
||||
maybe_emit_implicit_skill_invocation(
|
||||
session.as_ref(),
|
||||
turn.as_ref(),
|
||||
|
||||
@@ -25,6 +25,7 @@ use crate::unified_exec::UnifiedExecContext;
|
||||
use crate::unified_exec::UnifiedExecError;
|
||||
use crate::unified_exec::UnifiedExecProcessManager;
|
||||
use crate::unified_exec::generate_chunk_id;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_features::Feature;
|
||||
use codex_otel::SessionTelemetry;
|
||||
use codex_otel::TOOL_CALL_UNIFIED_EXEC_METRIC;
|
||||
@@ -136,7 +137,7 @@ impl ToolExecutor<ToolInvocation> for ExecCommandHandler {
|
||||
session.as_ref(),
|
||||
context.turn.as_ref(),
|
||||
&hook_command,
|
||||
&cwd,
|
||||
&EnvironmentPathRef::new(Arc::clone(&fs), cwd.clone()),
|
||||
)
|
||||
.await;
|
||||
let process_id = manager.allocate_process_id().await;
|
||||
|
||||
@@ -41,19 +41,28 @@ use crate::protocol::ExecExitedNotification;
|
||||
use crate::protocol::ExecOutputDeltaNotification;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::ExecResponse;
|
||||
use crate::protocol::FS_CANONICALIZE_METHOD;
|
||||
use crate::protocol::FS_COPY_METHOD;
|
||||
use crate::protocol::FS_CREATE_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_METHOD;
|
||||
use crate::protocol::FS_JOIN_METHOD;
|
||||
use crate::protocol::FS_PARENT_METHOD;
|
||||
use crate::protocol::FS_READ_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_READ_FILE_METHOD;
|
||||
use crate::protocol::FS_REMOVE_METHOD;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsJoinParams;
|
||||
use crate::protocol::FsJoinResponse;
|
||||
use crate::protocol::FsParentParams;
|
||||
use crate::protocol::FsParentResponse;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadDirectoryResponse;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
@@ -414,6 +423,24 @@ impl ExecServerClient {
|
||||
self.call(FS_GET_METADATA_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
) -> Result<FsCanonicalizeResponse, ExecServerError> {
|
||||
self.call(FS_CANONICALIZE_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_join(&self, params: FsJoinParams) -> Result<FsJoinResponse, ExecServerError> {
|
||||
self.call(FS_JOIN_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_parent(
|
||||
&self,
|
||||
params: FsParentParams,
|
||||
) -> Result<FsParentResponse, ExecServerError> {
|
||||
self.call(FS_PARENT_METHOD, ¶ms).await
|
||||
}
|
||||
|
||||
pub async fn fs_read_directory(
|
||||
&self,
|
||||
params: FsReadDirectoryParams,
|
||||
|
||||
@@ -2,6 +2,7 @@ use std::fmt;
|
||||
use std::hash::Hash;
|
||||
use std::hash::Hasher;
|
||||
use std::io;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -20,22 +21,27 @@ pub struct EnvironmentPathRef {
|
||||
}
|
||||
|
||||
impl EnvironmentPathRef {
|
||||
/// Creates a path ref bound to the filesystem that owns `path`.
|
||||
pub fn new(file_system: Arc<dyn ExecutorFileSystem>, path: AbsolutePathBuf) -> Self {
|
||||
Self { file_system, path }
|
||||
}
|
||||
|
||||
/// Creates a path ref bound to the shared unsandboxed local filesystem.
|
||||
pub fn local(path: AbsolutePathBuf) -> Self {
|
||||
Self::new(Arc::clone(&LOCAL_FS), path)
|
||||
}
|
||||
|
||||
/// Returns the absolute path held by this ref.
|
||||
pub fn path(&self) -> &AbsolutePathBuf {
|
||||
&self.path
|
||||
}
|
||||
|
||||
/// Returns the filesystem that owns this path.
|
||||
pub fn file_system(&self) -> Arc<dyn ExecutorFileSystem> {
|
||||
Arc::clone(&self.file_system)
|
||||
}
|
||||
|
||||
/// Reads this path as UTF-8 text through its bound filesystem.
|
||||
pub async fn read_to_string(
|
||||
&self,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
@@ -43,6 +49,7 @@ impl EnvironmentPathRef {
|
||||
self.file_system.read_file_text(&self.path, sandbox).await
|
||||
}
|
||||
|
||||
/// Reads metadata for this path through its bound filesystem.
|
||||
pub async fn metadata(
|
||||
&self,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
@@ -50,6 +57,7 @@ impl EnvironmentPathRef {
|
||||
self.file_system.get_metadata(&self.path, sandbox).await
|
||||
}
|
||||
|
||||
/// Reads directory entries for this path through its bound filesystem.
|
||||
pub async fn read_directory(
|
||||
&self,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
@@ -57,9 +65,37 @@ impl EnvironmentPathRef {
|
||||
self.file_system.read_directory(&self.path, sandbox).await
|
||||
}
|
||||
|
||||
/// Returns a ref with the same filesystem and a replacement path.
|
||||
pub fn with_path(&self, path: AbsolutePathBuf) -> Self {
|
||||
Self::new(Arc::clone(&self.file_system), path)
|
||||
}
|
||||
|
||||
/// Lexically joins `path` onto this path through its bound filesystem.
|
||||
pub async fn join<P: AsRef<Path>>(&self, path: P) -> io::Result<Self> {
|
||||
self.file_system
|
||||
.join(&self.path, path.as_ref())
|
||||
.await
|
||||
.map(|path| self.with_path(path))
|
||||
}
|
||||
|
||||
/// Returns the parent of this path through its bound filesystem.
|
||||
pub async fn parent(&self) -> io::Result<Option<Self>> {
|
||||
self.file_system
|
||||
.parent(&self.path)
|
||||
.await
|
||||
.map(|path| path.map(|path| self.with_path(path)))
|
||||
}
|
||||
|
||||
/// Canonicalizes this path through its bound filesystem.
|
||||
pub async fn canonicalize(
|
||||
&self,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> io::Result<Self> {
|
||||
self.file_system
|
||||
.canonicalize(&self.path, sandbox)
|
||||
.await
|
||||
.map(|path| self.with_path(path))
|
||||
}
|
||||
}
|
||||
|
||||
impl PartialEq for EnvironmentPathRef {
|
||||
@@ -96,10 +132,16 @@ mod tests {
|
||||
use codex_utils_absolute_path::test_support::PathBufExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashSet;
|
||||
use std::path::Path;
|
||||
use std::sync::Mutex;
|
||||
|
||||
use crate::LOCAL_FS;
|
||||
|
||||
#[derive(Clone, Debug, Eq, PartialEq)]
|
||||
enum RecordedMethod {
|
||||
Canonicalize,
|
||||
Join,
|
||||
Parent,
|
||||
ReadFileText,
|
||||
Metadata,
|
||||
ReadDirectory,
|
||||
@@ -140,8 +182,47 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
fn local_path_ref(path: AbsolutePathBuf) -> EnvironmentPathRef {
|
||||
EnvironmentPathRef::new(Arc::clone(&LOCAL_FS), path)
|
||||
}
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for RecordingFileSystem {
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
_sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> io::Result<AbsolutePathBuf> {
|
||||
self.push_call(RecordedCall {
|
||||
method: RecordedMethod::Canonicalize,
|
||||
path: path.clone(),
|
||||
sandbox: None,
|
||||
});
|
||||
Ok(path.parent().unwrap())
|
||||
}
|
||||
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> io::Result<AbsolutePathBuf> {
|
||||
self.push_call(RecordedCall {
|
||||
method: RecordedMethod::Join,
|
||||
path: base_path.clone(),
|
||||
sandbox: None,
|
||||
});
|
||||
AbsolutePathBuf::from_absolute_path_checked(base_path.as_path().join(path))
|
||||
}
|
||||
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> io::Result<Option<AbsolutePathBuf>> {
|
||||
self.push_call(RecordedCall {
|
||||
method: RecordedMethod::Parent,
|
||||
path: path.clone(),
|
||||
sandbox: None,
|
||||
});
|
||||
Ok(path.parent())
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
@@ -318,4 +399,129 @@ mod tests {
|
||||
let set = HashSet::from([left, same, different_path, different_fs]);
|
||||
assert_eq!(set.len(), 3);
|
||||
}
|
||||
#[tokio::test]
|
||||
async fn canonicalize_keeps_bound_file_system_identity() {
|
||||
let path = std::env::temp_dir().join("skills/demo").abs();
|
||||
let file_system = Arc::new(RecordingFileSystem::default());
|
||||
let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone());
|
||||
|
||||
let canonicalized = path_ref
|
||||
.canonicalize(/*sandbox*/ None)
|
||||
.await
|
||||
.expect("canonicalize");
|
||||
|
||||
assert_eq!(canonicalized.path(), &path.parent().unwrap());
|
||||
assert_eq!(
|
||||
canonicalized,
|
||||
EnvironmentPathRef::new(file_system.clone(), path.parent().unwrap())
|
||||
);
|
||||
assert_eq!(
|
||||
file_system.recorded_calls(),
|
||||
vec![RecordedCall {
|
||||
method: RecordedMethod::Canonicalize,
|
||||
path,
|
||||
sandbox: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn join_keeps_bound_file_system_identity() {
|
||||
let path = std::env::temp_dir().join("skills").abs();
|
||||
let file_system = Arc::new(RecordingFileSystem::default());
|
||||
let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone());
|
||||
|
||||
assert_eq!(
|
||||
path_ref.join(Path::new("demo")).await.ok(),
|
||||
Some(EnvironmentPathRef::new(
|
||||
file_system.clone(),
|
||||
std::env::temp_dir().join("skills/demo").abs(),
|
||||
))
|
||||
);
|
||||
assert_eq!(
|
||||
file_system.recorded_calls(),
|
||||
vec![RecordedCall {
|
||||
method: RecordedMethod::Join,
|
||||
path,
|
||||
sandbox: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn join_matches_absolute_path_buf_for_tilde_paths() {
|
||||
let path_ref = local_path_ref(std::env::temp_dir().join("skills").abs());
|
||||
|
||||
assert_eq!(
|
||||
path_ref
|
||||
.join(Path::new("~"))
|
||||
.await
|
||||
.ok()
|
||||
.map(|path_ref| path_ref.path().clone()),
|
||||
Some(path_ref.path().join(Path::new("~")))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn join_matches_absolute_path_buf_for_parent_dirs() {
|
||||
let path_ref = local_path_ref(std::env::temp_dir().join("skills").abs());
|
||||
|
||||
assert_eq!(
|
||||
path_ref
|
||||
.join(Path::new("../outside"))
|
||||
.await
|
||||
.expect("join")
|
||||
.path()
|
||||
.clone(),
|
||||
path_ref.path().join(Path::new("../outside"))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn parent_keeps_bound_file_system_identity() {
|
||||
let path = std::env::temp_dir().join("skills/demo").abs();
|
||||
let file_system = Arc::new(RecordingFileSystem::default());
|
||||
let path_ref = EnvironmentPathRef::new(file_system.clone(), path.clone());
|
||||
|
||||
assert_eq!(
|
||||
path_ref.parent().await.expect("parent"),
|
||||
Some(EnvironmentPathRef::new(
|
||||
file_system.clone(),
|
||||
std::env::temp_dir().join("skills").abs(),
|
||||
))
|
||||
);
|
||||
assert_eq!(
|
||||
file_system.recorded_calls(),
|
||||
vec![RecordedCall {
|
||||
method: RecordedMethod::Parent,
|
||||
path,
|
||||
sandbox: None,
|
||||
}]
|
||||
);
|
||||
}
|
||||
|
||||
#[cfg(windows)]
|
||||
#[tokio::test]
|
||||
async fn join_matches_absolute_path_buf_for_windows_prefixed_and_rooted_paths() {
|
||||
let path_ref = local_path_ref(std::env::temp_dir().join("skills").abs());
|
||||
|
||||
assert_eq!(
|
||||
path_ref
|
||||
.join(Path::new(r"C:temp"))
|
||||
.await
|
||||
.expect("join")
|
||||
.path()
|
||||
.clone(),
|
||||
path_ref.path().join(Path::new(r"C:temp"))
|
||||
);
|
||||
assert_eq!(
|
||||
path_ref
|
||||
.join(Path::new(r"\temp"))
|
||||
.await
|
||||
.expect("join")
|
||||
.path()
|
||||
.clone(),
|
||||
path_ref.path().join(Path::new(r"\temp"))
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -10,6 +10,7 @@ use crate::CreateDirectoryOptions;
|
||||
use crate::ExecutorFileSystem;
|
||||
use crate::RemoveOptions;
|
||||
use crate::local_file_system::DirectFileSystem;
|
||||
use crate::protocol::FS_CANONICALIZE_METHOD;
|
||||
use crate::protocol::FS_COPY_METHOD;
|
||||
use crate::protocol::FS_CREATE_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_METHOD;
|
||||
@@ -17,6 +18,8 @@ use crate::protocol::FS_READ_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_READ_FILE_METHOD;
|
||||
use crate::protocol::FS_REMOVE_METHOD;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
@@ -49,6 +52,8 @@ pub(crate) enum FsHelperRequest {
|
||||
CreateDirectory(FsCreateDirectoryParams),
|
||||
#[serde(rename = "fs/getMetadata")]
|
||||
GetMetadata(FsGetMetadataParams),
|
||||
#[serde(rename = "fs/canonicalize")]
|
||||
Canonicalize(FsCanonicalizeParams),
|
||||
#[serde(rename = "fs/readDirectory")]
|
||||
ReadDirectory(FsReadDirectoryParams),
|
||||
#[serde(rename = "fs/remove")]
|
||||
@@ -75,6 +80,8 @@ pub(crate) enum FsHelperPayload {
|
||||
CreateDirectory(FsCreateDirectoryResponse),
|
||||
#[serde(rename = "fs/getMetadata")]
|
||||
GetMetadata(FsGetMetadataResponse),
|
||||
#[serde(rename = "fs/canonicalize")]
|
||||
Canonicalize(FsCanonicalizeResponse),
|
||||
#[serde(rename = "fs/readDirectory")]
|
||||
ReadDirectory(FsReadDirectoryResponse),
|
||||
#[serde(rename = "fs/remove")]
|
||||
@@ -90,6 +97,7 @@ impl FsHelperPayload {
|
||||
Self::WriteFile(_) => FS_WRITE_FILE_METHOD,
|
||||
Self::CreateDirectory(_) => FS_CREATE_DIRECTORY_METHOD,
|
||||
Self::GetMetadata(_) => FS_GET_METADATA_METHOD,
|
||||
Self::Canonicalize(_) => FS_CANONICALIZE_METHOD,
|
||||
Self::ReadDirectory(_) => FS_READ_DIRECTORY_METHOD,
|
||||
Self::Remove(_) => FS_REMOVE_METHOD,
|
||||
Self::Copy(_) => FS_COPY_METHOD,
|
||||
@@ -132,6 +140,16 @@ impl FsHelperPayload {
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn expect_canonicalize(self) -> Result<FsCanonicalizeResponse, JSONRPCErrorError> {
|
||||
match self {
|
||||
Self::Canonicalize(response) => Ok(response),
|
||||
other => Err(unexpected_response(
|
||||
FS_CANONICALIZE_METHOD,
|
||||
other.operation(),
|
||||
)),
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn expect_read_directory(
|
||||
self,
|
||||
) -> Result<FsReadDirectoryResponse, JSONRPCErrorError> {
|
||||
@@ -219,6 +237,15 @@ pub(crate) async fn run_direct_request(
|
||||
modified_at_ms: metadata.modified_at_ms,
|
||||
}))
|
||||
}
|
||||
FsHelperRequest::Canonicalize(params) => {
|
||||
let path = file_system
|
||||
.canonicalize(¶ms.path, /*sandbox*/ None)
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsHelperPayload::Canonicalize(FsCanonicalizeResponse {
|
||||
path,
|
||||
}))
|
||||
}
|
||||
FsHelperRequest::ReadDirectory(params) => {
|
||||
let entries = file_system
|
||||
.read_directory(¶ms.path, /*sandbox*/ None)
|
||||
|
||||
@@ -64,12 +64,18 @@ pub use protocol::ExecOutputDeltaNotification;
|
||||
pub use protocol::ExecOutputStream;
|
||||
pub use protocol::ExecParams;
|
||||
pub use protocol::ExecResponse;
|
||||
pub use protocol::FsCanonicalizeParams;
|
||||
pub use protocol::FsCanonicalizeResponse;
|
||||
pub use protocol::FsCopyParams;
|
||||
pub use protocol::FsCopyResponse;
|
||||
pub use protocol::FsCreateDirectoryParams;
|
||||
pub use protocol::FsCreateDirectoryResponse;
|
||||
pub use protocol::FsGetMetadataParams;
|
||||
pub use protocol::FsGetMetadataResponse;
|
||||
pub use protocol::FsJoinParams;
|
||||
pub use protocol::FsJoinResponse;
|
||||
pub use protocol::FsParentParams;
|
||||
pub use protocol::FsParentResponse;
|
||||
pub use protocol::FsReadDirectoryEntry;
|
||||
pub use protocol::FsReadDirectoryParams;
|
||||
pub use protocol::FsReadDirectoryResponse;
|
||||
|
||||
@@ -79,6 +79,27 @@ impl LocalFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for LocalFileSystem {
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
let (file_system, sandbox) = self.file_system_for(sandbox)?;
|
||||
file_system.canonicalize(path, sandbox).await
|
||||
}
|
||||
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
self.unsandboxed.join(base_path, path).await
|
||||
}
|
||||
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult<Option<AbsolutePathBuf>> {
|
||||
self.unsandboxed.parent(path).await
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
@@ -152,6 +173,27 @@ impl ExecutorFileSystem for LocalFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for UnsandboxedFileSystem {
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
reject_platform_sandbox_context(sandbox)?;
|
||||
self.file_system.canonicalize(path, /*sandbox*/ None).await
|
||||
}
|
||||
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
self.file_system.join(base_path, path).await
|
||||
}
|
||||
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult<Option<AbsolutePathBuf>> {
|
||||
self.file_system.parent(path).await
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
@@ -238,6 +280,27 @@ impl ExecutorFileSystem for UnsandboxedFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for DirectFileSystem {
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
reject_sandbox_context(sandbox)?;
|
||||
AbsolutePathBuf::from_absolute_path(tokio::fs::canonicalize(path.as_path()).await?)
|
||||
}
|
||||
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
Ok(base_path.join(path))
|
||||
}
|
||||
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult<Option<AbsolutePathBuf>> {
|
||||
Ok(path.parent())
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
@@ -23,6 +23,9 @@ pub const FS_READ_FILE_METHOD: &str = "fs/readFile";
|
||||
pub const FS_WRITE_FILE_METHOD: &str = "fs/writeFile";
|
||||
pub const FS_CREATE_DIRECTORY_METHOD: &str = "fs/createDirectory";
|
||||
pub const FS_GET_METADATA_METHOD: &str = "fs/getMetadata";
|
||||
pub const FS_CANONICALIZE_METHOD: &str = "fs/canonicalize";
|
||||
pub const FS_JOIN_METHOD: &str = "fs/join";
|
||||
pub const FS_PARENT_METHOD: &str = "fs/parent";
|
||||
pub const FS_READ_DIRECTORY_METHOD: &str = "fs/readDirectory";
|
||||
pub const FS_REMOVE_METHOD: &str = "fs/remove";
|
||||
pub const FS_COPY_METHOD: &str = "fs/copy";
|
||||
@@ -211,6 +214,44 @@ pub struct FsGetMetadataResponse {
|
||||
pub modified_at_ms: i64,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsCanonicalizeParams {
|
||||
pub path: AbsolutePathBuf,
|
||||
pub sandbox: Option<FileSystemSandboxContext>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsCanonicalizeResponse {
|
||||
pub path: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsJoinParams {
|
||||
pub base_path: AbsolutePathBuf,
|
||||
pub path: PathBuf,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsJoinResponse {
|
||||
pub path: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsParentParams {
|
||||
pub path: AbsolutePathBuf,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsParentResponse {
|
||||
pub path: Option<AbsolutePathBuf>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
pub struct FsReadDirectoryParams {
|
||||
|
||||
@@ -2,6 +2,7 @@ use async_trait::async_trait;
|
||||
use base64::Engine as _;
|
||||
use base64::engine::general_purpose::STANDARD;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::path::Path;
|
||||
use tokio::io;
|
||||
use tracing::trace;
|
||||
|
||||
@@ -15,9 +16,12 @@ use crate::FileSystemSandboxContext;
|
||||
use crate::ReadDirectoryEntry;
|
||||
use crate::RemoveOptions;
|
||||
use crate::client::LazyRemoteExecServerClient;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsJoinParams;
|
||||
use crate::protocol::FsParentParams;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
use crate::protocol::FsRemoveParams;
|
||||
@@ -26,7 +30,6 @@ use crate::protocol::FsWriteFileParams;
|
||||
const INVALID_REQUEST_ERROR_CODE: i64 = -32600;
|
||||
const NOT_FOUND_ERROR_CODE: i64 = -32004;
|
||||
|
||||
#[derive(Clone)]
|
||||
pub(crate) struct RemoteFileSystem {
|
||||
client: LazyRemoteExecServerClient,
|
||||
}
|
||||
@@ -40,6 +43,50 @@ impl RemoteFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for RemoteFileSystem {
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
trace!("remote fs canonicalize");
|
||||
let client = self.client.get().await.map_err(map_remote_error)?;
|
||||
let response = client
|
||||
.fs_canonicalize(FsCanonicalizeParams {
|
||||
path: path.clone(),
|
||||
sandbox: remote_sandbox_context(sandbox),
|
||||
})
|
||||
.await
|
||||
.map_err(map_remote_error)?;
|
||||
Ok(response.path)
|
||||
}
|
||||
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
trace!("remote fs join");
|
||||
let client = self.client.get().await.map_err(map_remote_error)?;
|
||||
let response = client
|
||||
.fs_join(FsJoinParams {
|
||||
base_path: base_path.clone(),
|
||||
path: path.to_path_buf(),
|
||||
})
|
||||
.await
|
||||
.map_err(map_remote_error)?;
|
||||
Ok(response.path)
|
||||
}
|
||||
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult<Option<AbsolutePathBuf>> {
|
||||
trace!("remote fs parent");
|
||||
let client = self.client.get().await.map_err(map_remote_error)?;
|
||||
let response = client
|
||||
.fs_parent(FsParentParams { path: path.clone() })
|
||||
.await
|
||||
.map_err(map_remote_error)?;
|
||||
Ok(response.path)
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
@@ -3,6 +3,7 @@ use base64::Engine as _;
|
||||
use base64::engine::general_purpose::STANDARD;
|
||||
use codex_app_server_protocol::JSONRPCErrorError;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use std::path::Path;
|
||||
use tokio::io;
|
||||
|
||||
use crate::CopyOptions;
|
||||
@@ -17,6 +18,7 @@ use crate::RemoveOptions;
|
||||
use crate::fs_helper::FsHelperPayload;
|
||||
use crate::fs_helper::FsHelperRequest;
|
||||
use crate::fs_sandbox::FileSystemSandboxRunner;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
@@ -51,6 +53,38 @@ impl SandboxedFileSystem {
|
||||
|
||||
#[async_trait]
|
||||
impl ExecutorFileSystem for SandboxedFileSystem {
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
let sandbox = require_platform_sandbox(sandbox)?;
|
||||
let response = self
|
||||
.run_sandboxed(
|
||||
sandbox,
|
||||
FsHelperRequest::Canonicalize(FsCanonicalizeParams {
|
||||
path: path.clone(),
|
||||
sandbox: None,
|
||||
}),
|
||||
)
|
||||
.await?
|
||||
.expect_canonicalize()
|
||||
.map_err(map_sandbox_error)?;
|
||||
Ok(response.path)
|
||||
}
|
||||
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> FileSystemResult<AbsolutePathBuf> {
|
||||
Ok(base_path.join(path))
|
||||
}
|
||||
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult<Option<AbsolutePathBuf>> {
|
||||
Ok(path.parent())
|
||||
}
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
@@ -11,12 +11,18 @@ use crate::ExecutorFileSystem;
|
||||
use crate::RemoveOptions;
|
||||
use crate::local_file_system::LocalFileSystem;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsJoinParams;
|
||||
use crate::protocol::FsJoinResponse;
|
||||
use crate::protocol::FsParentParams;
|
||||
use crate::protocol::FsParentResponse;
|
||||
use crate::protocol::FsReadDirectoryEntry;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadDirectoryResponse;
|
||||
@@ -106,6 +112,42 @@ impl FileSystemHandler {
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) async fn canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
) -> Result<FsCanonicalizeResponse, JSONRPCErrorError> {
|
||||
let path = self
|
||||
.file_system
|
||||
.canonicalize(¶ms.path, params.sandbox.as_ref())
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsCanonicalizeResponse { path })
|
||||
}
|
||||
|
||||
pub(crate) async fn join(
|
||||
&self,
|
||||
params: FsJoinParams,
|
||||
) -> Result<FsJoinResponse, JSONRPCErrorError> {
|
||||
let path = self
|
||||
.file_system
|
||||
.join(¶ms.base_path, ¶ms.path)
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsJoinResponse { path })
|
||||
}
|
||||
|
||||
pub(crate) async fn parent(
|
||||
&self,
|
||||
params: FsParentParams,
|
||||
) -> Result<FsParentResponse, JSONRPCErrorError> {
|
||||
let path = self
|
||||
.file_system
|
||||
.parent(¶ms.path)
|
||||
.await
|
||||
.map_err(map_fs_error)?;
|
||||
Ok(FsParentResponse { path })
|
||||
}
|
||||
|
||||
pub(crate) async fn read_directory(
|
||||
&self,
|
||||
params: FsReadDirectoryParams,
|
||||
|
||||
@@ -16,12 +16,18 @@ use crate::client::http_client::PendingReqwestHttpBodyStream;
|
||||
use crate::client::http_client::ReqwestHttpRequestRunner;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::ExecResponse;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCanonicalizeResponse;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCopyResponse;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsCreateDirectoryResponse;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsGetMetadataResponse;
|
||||
use crate::protocol::FsJoinParams;
|
||||
use crate::protocol::FsJoinResponse;
|
||||
use crate::protocol::FsParentParams;
|
||||
use crate::protocol::FsParentResponse;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadDirectoryResponse;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
@@ -240,6 +246,30 @@ impl ExecServerHandler {
|
||||
self.file_system.get_metadata(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_canonicalize(
|
||||
&self,
|
||||
params: FsCanonicalizeParams,
|
||||
) -> Result<FsCanonicalizeResponse, JSONRPCErrorError> {
|
||||
self.require_initialized_for("filesystem")?;
|
||||
self.file_system.canonicalize(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_join(
|
||||
&self,
|
||||
params: FsJoinParams,
|
||||
) -> Result<FsJoinResponse, JSONRPCErrorError> {
|
||||
self.require_initialized_for("filesystem")?;
|
||||
self.file_system.join(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_parent(
|
||||
&self,
|
||||
params: FsParentParams,
|
||||
) -> Result<FsParentResponse, JSONRPCErrorError> {
|
||||
self.require_initialized_for("filesystem")?;
|
||||
self.file_system.parent(params).await
|
||||
}
|
||||
|
||||
pub(crate) async fn fs_read_directory(
|
||||
&self,
|
||||
params: FsReadDirectoryParams,
|
||||
|
||||
@@ -5,16 +5,22 @@ use crate::protocol::EXEC_READ_METHOD;
|
||||
use crate::protocol::EXEC_TERMINATE_METHOD;
|
||||
use crate::protocol::EXEC_WRITE_METHOD;
|
||||
use crate::protocol::ExecParams;
|
||||
use crate::protocol::FS_CANONICALIZE_METHOD;
|
||||
use crate::protocol::FS_COPY_METHOD;
|
||||
use crate::protocol::FS_CREATE_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_GET_METADATA_METHOD;
|
||||
use crate::protocol::FS_JOIN_METHOD;
|
||||
use crate::protocol::FS_PARENT_METHOD;
|
||||
use crate::protocol::FS_READ_DIRECTORY_METHOD;
|
||||
use crate::protocol::FS_READ_FILE_METHOD;
|
||||
use crate::protocol::FS_REMOVE_METHOD;
|
||||
use crate::protocol::FS_WRITE_FILE_METHOD;
|
||||
use crate::protocol::FsCanonicalizeParams;
|
||||
use crate::protocol::FsCopyParams;
|
||||
use crate::protocol::FsCreateDirectoryParams;
|
||||
use crate::protocol::FsGetMetadataParams;
|
||||
use crate::protocol::FsJoinParams;
|
||||
use crate::protocol::FsParentParams;
|
||||
use crate::protocol::FsReadDirectoryParams;
|
||||
use crate::protocol::FsReadFileParams;
|
||||
use crate::protocol::FsRemoveParams;
|
||||
@@ -96,6 +102,24 @@ pub(crate) fn build_router() -> RpcRouter<ExecServerHandler> {
|
||||
handler.fs_get_metadata(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_CANONICALIZE_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsCanonicalizeParams| async move {
|
||||
handler.fs_canonicalize(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_JOIN_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsJoinParams| async move {
|
||||
handler.fs_join(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_PARENT_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsParentParams| async move {
|
||||
handler.fs_parent(params).await
|
||||
},
|
||||
);
|
||||
router.request(
|
||||
FS_READ_DIRECTORY_METHOD,
|
||||
|handler: Arc<ExecServerHandler>, params: FsReadDirectoryParams| async move {
|
||||
|
||||
@@ -365,6 +365,49 @@ async fn file_system_methods_cover_surface_area(use_remote: bool) -> Result<()>
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
|
||||
let source_link = tmp.path().join("source-link");
|
||||
symlink(&source_dir, &source_link)?;
|
||||
let joined_nested = file_system
|
||||
.join(
|
||||
&absolute_path(source_link.clone()),
|
||||
Path::new("nested/note.txt"),
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(
|
||||
joined_nested,
|
||||
absolute_path(source_link.join("nested").join("note.txt"))
|
||||
);
|
||||
let joined_parent = file_system
|
||||
.parent(&joined_nested)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(
|
||||
joined_parent,
|
||||
Some(absolute_path(source_link.join("nested")))
|
||||
);
|
||||
let joined_parent_traversal = file_system
|
||||
.join(&absolute_path(source_dir.clone()), Path::new("../outside"))
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(
|
||||
joined_parent_traversal,
|
||||
absolute_path(source_dir.join("../outside"))
|
||||
);
|
||||
let canonical_nested = file_system
|
||||
.canonicalize(
|
||||
&absolute_path(source_link.join("nested").join("note.txt")),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(
|
||||
canonical_nested,
|
||||
absolute_path(std::fs::canonicalize(
|
||||
source_dir.join("nested").join("note.txt")
|
||||
)?)
|
||||
);
|
||||
|
||||
let nested_file_contents = file_system
|
||||
.read_file(&absolute_path(nested_file.clone()), /*sandbox*/ None)
|
||||
.await
|
||||
@@ -530,6 +573,32 @@ async fn file_system_sandboxed_read_allows_readable_root(use_remote: bool) -> Re
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn file_system_sandboxed_canonicalize_allows_readable_root(use_remote: bool) -> Result<()> {
|
||||
let context = create_file_system_context(use_remote).await?;
|
||||
let file_system = context.file_system;
|
||||
|
||||
let tmp = TempDir::new()?;
|
||||
let allowed_dir = tmp.path().join("allowed");
|
||||
let file_path = allowed_dir.join("note.txt");
|
||||
std::fs::create_dir_all(&allowed_dir)?;
|
||||
std::fs::write(&file_path, "sandboxed hello")?;
|
||||
let sandbox = read_only_sandbox(allowed_dir);
|
||||
|
||||
let canonical_path = file_system
|
||||
.canonicalize(&absolute_path(file_path.clone()), Some(&sandbox))
|
||||
.await
|
||||
.with_context(|| format!("mode={use_remote}"))?;
|
||||
assert_eq!(
|
||||
canonical_path,
|
||||
absolute_path(std::fs::canonicalize(file_path)?)
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test_case(false ; "local")]
|
||||
#[test_case(true ; "remote")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
|
||||
@@ -133,6 +133,23 @@ pub type FileSystemResult<T> = io::Result<T>;
|
||||
/// a remote environment.
|
||||
#[async_trait]
|
||||
pub trait ExecutorFileSystem: Send + Sync {
|
||||
/// Resolves a path within this filesystem.
|
||||
async fn canonicalize(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
sandbox: Option<&FileSystemSandboxContext>,
|
||||
) -> FileSystemResult<AbsolutePathBuf>;
|
||||
|
||||
/// Lexically joins a path onto an existing bound path.
|
||||
async fn join(
|
||||
&self,
|
||||
base_path: &AbsolutePathBuf,
|
||||
path: &Path,
|
||||
) -> FileSystemResult<AbsolutePathBuf>;
|
||||
|
||||
/// Returns the parent directory of a bound path.
|
||||
async fn parent(&self, path: &AbsolutePathBuf) -> FileSystemResult<Option<AbsolutePathBuf>>;
|
||||
|
||||
async fn read_file(
|
||||
&self,
|
||||
path: &AbsolutePathBuf,
|
||||
|
||||
@@ -4461,6 +4461,7 @@ impl ChatComposer {
|
||||
mod tests {
|
||||
use super::attachment_state::AttachedImage;
|
||||
use super::*;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use crate::test_support::PathBufExt;
|
||||
use crate::test_support::test_path_buf;
|
||||
use image::ImageBuffer;
|
||||
@@ -6254,6 +6255,7 @@ mod tests {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(skill_path.clone()),
|
||||
path_to_skills_md: skill_path.clone(),
|
||||
scope: crate::test_support::skill_scope_user(),
|
||||
plugin_id: None,
|
||||
@@ -6297,6 +6299,7 @@ mod tests {
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(skill_path.clone()),
|
||||
path_to_skills_md: skill_path.clone(),
|
||||
scope: crate::test_support::skill_scope_repo(),
|
||||
plugin_id: None,
|
||||
@@ -6389,6 +6392,7 @@ mod tests {
|
||||
}),
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/repo/google-calendar/SKILL.md").abs(),
|
||||
scope: crate::test_support::skill_scope_repo(),
|
||||
plugin_id: None,
|
||||
|
||||
@@ -1787,6 +1787,7 @@ impl Renderable for BottomPane {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use crate::app::app_server_requests::ResolvedAppServerRequest;
|
||||
use crate::app_command::AppCommand as Op;
|
||||
use crate::app_event::AppEvent;
|
||||
@@ -2576,6 +2577,7 @@ mod tests {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(test_path_buf("/tmp/test-skill/SKILL.md").abs()),
|
||||
path_to_skills_md: test_path_buf("/tmp/test-skill/SKILL.md").abs(),
|
||||
scope: crate::test_support::skill_scope_user(),
|
||||
plugin_id: None,
|
||||
|
||||
@@ -18,6 +18,7 @@ use codex_core_skills::model::SkillDependencies;
|
||||
use codex_core_skills::model::SkillInterface;
|
||||
use codex_core_skills::model::SkillMetadata;
|
||||
use codex_core_skills::model::SkillToolDependency;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::parse_command::ParsedCommand;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -241,6 +242,9 @@ fn protocol_skill_to_core(skill: &ProtocolSkillMetadata) -> Option<SkillMetadata
|
||||
.collect(),
|
||||
}),
|
||||
policy: None,
|
||||
// TUI only uses listed skills for display and raw-path mention payloads. `skills/list`
|
||||
// does not carry source authority yet, so this placeholder must not be used for reads.
|
||||
source_path: EnvironmentPathRef::local(skill.path.clone()),
|
||||
path_to_skills_md: skill.path.clone(),
|
||||
scope,
|
||||
plugin_id: None,
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use super::*;
|
||||
use crate::app_event::ConnectorsSnapshot;
|
||||
use codex_exec_server::EnvironmentPathRef;
|
||||
use codex_protocol::models::ManagedFileSystemPermissions;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
@@ -549,6 +550,7 @@ async fn submission_prefers_selected_duplicate_skill_path() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(repo_skill_path.clone()),
|
||||
path_to_skills_md: repo_skill_path,
|
||||
scope: crate::test_support::skill_scope_repo(),
|
||||
plugin_id: None,
|
||||
@@ -560,6 +562,7 @@ async fn submission_prefers_selected_duplicate_skill_path() {
|
||||
interface: None,
|
||||
dependencies: None,
|
||||
policy: None,
|
||||
source_path: EnvironmentPathRef::local(user_skill_path.clone()),
|
||||
path_to_skills_md: user_skill_path.clone(),
|
||||
scope: crate::test_support::skill_scope_user(),
|
||||
plugin_id: None,
|
||||
|
||||
Reference in New Issue
Block a user