mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
Enforce current filesystem permissions when reading repository skills (#59278)
## Why Cached repository skill metadata must not authorize reads after access is revoked or a skill file or directory becomes a symlink to a denied location. ## What changed Read repository skill instructions through a current environment accessor for the filesystem that discovered them. Reject reads when no matching accessor is available, and cache filesystem identity instead of retaining unrestricted read access. ## Testing Add regression coverage for revoked permissions, file and directory symlink replacements, and missing or mismatched filesystem accessors. GitOrigin-RevId: 8dad83b7c7893e0d66e2fe1c0c240715db582005
This commit is contained in:
committed by
copyberry
parent
f39e45e4a6
commit
afefd0ba95
@@ -84,6 +84,7 @@ use codex_extension_api::ExtensionData;
|
||||
use codex_extension_api::TurnInputContext;
|
||||
use codex_extension_api::TurnInputEnvironment;
|
||||
use codex_features::Feature;
|
||||
use codex_file_system::EnvironmentAccess;
|
||||
use codex_file_system::FindUpErrorPolicy;
|
||||
use codex_file_system::find_nearest_ancestor_with_markers;
|
||||
use codex_hooks::ModelRequestHookEvent::PostModelRequest;
|
||||
@@ -1270,11 +1271,18 @@ async fn build_skills_and_plugins(
|
||||
let injected_host_skill_prompts = turn_context
|
||||
.extension_data
|
||||
.get::<InjectedHostSkillPrompts>();
|
||||
let environments = step_context.environments();
|
||||
let accessors = environments
|
||||
.iter()
|
||||
.map(|(_, access)| access as &dyn EnvironmentAccess)
|
||||
.collect::<Vec<_>>();
|
||||
let HostSkillPrompts {
|
||||
fragments,
|
||||
injected: injected_host_skills,
|
||||
warnings: host_skill_warnings,
|
||||
} = skills_snapshot.load_skill_prompts(&mentioned_skills).await;
|
||||
} = skills_snapshot
|
||||
.load_skill_prompts(&mentioned_skills, &accessors)
|
||||
.await;
|
||||
emit_explicit_skill_invocations(
|
||||
sess,
|
||||
turn_context,
|
||||
|
||||
@@ -36,7 +36,6 @@ use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::skip_if_remote;
|
||||
use core_test_support::skip_if_wine_exec;
|
||||
use core_test_support::test_codex::local_requests;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::test_codex::turn_permission_fields;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -148,7 +147,6 @@ async fn user_turn_includes_skill_instructions() -> Result<()> {
|
||||
},
|
||||
])
|
||||
.with_thread_settings(ThreadSettingsOverrides {
|
||||
environments: Some(local_requests(test.config.cwd.clone())),
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
sandbox_policy: Some(sandbox_policy),
|
||||
permission_profile,
|
||||
|
||||
@@ -1,3 +1,7 @@
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::FileSystemSandboxContext;
|
||||
use codex_skills_extension::HostSkillsLoadInput;
|
||||
use core_test_support::test_codex::TestCodexHarness;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
@@ -774,6 +778,284 @@ async fn capability_sections_render_in_order_with_host_repo_and_plugin_skills()
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
enum CachedRepositorySkillChange {
|
||||
ReplaceFileWithSymlink,
|
||||
ReplaceDirectoryWithSymlink,
|
||||
}
|
||||
|
||||
/// Cached metadata never authorizes a live read after permissions or a skill's target changes.
|
||||
#[test_case(CachedRepositorySkillChange::ReplaceFileWithSymlink; "file becomes a symlink")]
|
||||
#[test_case(CachedRepositorySkillChange::ReplaceDirectoryWithSymlink; "parent becomes a symlink")]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn repository_skill_cache_cannot_read_inaccessible_files(
|
||||
change: CachedRepositorySkillChange,
|
||||
) -> Result<()> {
|
||||
skip_if_wine_exec!(
|
||||
Ok(()),
|
||||
"executor-backed repo skills require matching host and executor path conventions"
|
||||
);
|
||||
skip_if_target_windows!(
|
||||
Ok(()),
|
||||
"restricted reads require the elevated Windows sandbox backend, unavailable in this fixture"
|
||||
);
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
const GATED_BODY: &str = "The gated skill's private instructions.";
|
||||
let mut extensions = ExtensionRegistryBuilder::<Config>::new();
|
||||
install(&mut extensions, |config: &Config| SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
max_context_tokens: config.skill_max_context_tokens,
|
||||
catalog_deltas_enabled: config.features.enabled(Feature::SkillCatalogDeltas),
|
||||
omit_descriptions: false,
|
||||
bundled_skills_enabled: false,
|
||||
cloud_skill_enabled: false,
|
||||
shadow_selection_enabled: false,
|
||||
});
|
||||
let builder = test_codex()
|
||||
.with_extensions(Arc::new(extensions.build()))
|
||||
.with_config(configure_catalog_test)
|
||||
.with_workspace_setup(|cwd, file_system| async move {
|
||||
for (name, body) in [
|
||||
("repo-allowed", "The allowed skill's instructions."),
|
||||
("repo-gated", GATED_BODY),
|
||||
("repo-revoked", GATED_BODY),
|
||||
] {
|
||||
let directory =
|
||||
PathUri::from_host_native_path(cwd.join(".agents/skills").join(name))?;
|
||||
file_system
|
||||
.create_directory(
|
||||
&directory,
|
||||
CreateDirectoryOptions {
|
||||
recursive: true,
|
||||
follow_symlinks: true,
|
||||
},
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await?;
|
||||
file_system
|
||||
.write_file(
|
||||
&directory.join("SKILL.md")?,
|
||||
format!(
|
||||
"---\nname: {name}\ndescription: Repository skill.\n---\n\n{body}\n"
|
||||
)
|
||||
.into_bytes(),
|
||||
Default::default(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await?;
|
||||
}
|
||||
Ok(())
|
||||
});
|
||||
let harness = TestCodexHarness::with_auto_env_builder(builder).await?;
|
||||
let test = harness.test();
|
||||
let responses = responses::mount_sse_sequence(
|
||||
harness.server(),
|
||||
vec![
|
||||
sse(vec![ev_response_created("resp-1"), ev_completed("resp-1")]),
|
||||
sse(vec![ev_response_created("resp-2"), ev_completed("resp-2")]),
|
||||
sse(vec![ev_response_created("resp-3"), ev_completed("resp-3")]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
let gated_path = test
|
||||
.fs()
|
||||
.canonicalize(
|
||||
&PathUri::from_abs_path(&test.config.cwd.join(".agents/skills/repo-gated/SKILL.md")),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let selection = test.executor_environment().selection();
|
||||
let environment = test
|
||||
.thread_manager
|
||||
.environment_manager()
|
||||
.get_environment(&selection.environment_id)
|
||||
.expect("configured executor environment");
|
||||
let file_system = environment.get_filesystem();
|
||||
let executor_cwd = selection.cwd.clone();
|
||||
let denied_directory = executor_cwd.join("denied")?;
|
||||
harness.write_file("denied/SKILL.md", GATED_BODY).await?;
|
||||
let denied_directory = file_system
|
||||
.canonicalize(&denied_directory, /*sandbox*/ None)
|
||||
.await?;
|
||||
let initial_permissions = PermissionProfile::from_runtime_permissions(
|
||||
&FileSystemSandboxPolicy::restricted(vec![
|
||||
FileSystemSandboxEntry::new(
|
||||
FileSystemPath::Special {
|
||||
value: FileSystemSpecialPath::Root,
|
||||
},
|
||||
FileSystemAccessMode::Read,
|
||||
),
|
||||
FileSystemSandboxEntry::new(
|
||||
FileSystemPath::Path {
|
||||
path: denied_directory.clone(),
|
||||
},
|
||||
FileSystemAccessMode::Deny,
|
||||
),
|
||||
]),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
);
|
||||
let revoked_path = file_system
|
||||
.canonicalize(
|
||||
&executor_cwd.join(".agents/skills/repo-revoked/SKILL.md")?,
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await?;
|
||||
let (mut narrowed_policy, network_policy) = initial_permissions.to_runtime_permissions();
|
||||
narrowed_policy.entries.push(FileSystemSandboxEntry::new(
|
||||
FileSystemPath::Path { path: revoked_path },
|
||||
FileSystemAccessMode::Deny,
|
||||
));
|
||||
let narrowed_permissions =
|
||||
PermissionProfile::from_runtime_permissions(&narrowed_policy, network_policy);
|
||||
test.submit_turn_with_permission_profile("Inspect the repository skills.", initial_permissions)
|
||||
.await?;
|
||||
|
||||
let open = FileSystemEnvironmentAccessor::unrestricted(&file_system);
|
||||
let snapshot = test
|
||||
.thread_manager
|
||||
.skills_service()
|
||||
.snapshot_for_config(
|
||||
&HostSkillsLoadInput::new(
|
||||
test.config.cwd.clone(),
|
||||
Vec::new(),
|
||||
test.config.config_layer_stack.clone(),
|
||||
),
|
||||
Some(Arc::clone(&file_system)),
|
||||
)
|
||||
.await;
|
||||
let [revoked_skill, gated_skill] = ["repo-revoked", "repo-gated"].map(|name| {
|
||||
snapshot
|
||||
.outcome()
|
||||
.skills
|
||||
.iter()
|
||||
.find(|skill| skill.name == name)
|
||||
.expect("first turn discovered the repository skill")
|
||||
.clone()
|
||||
});
|
||||
|
||||
// Revoking access hides the skill and prevents cached metadata from reopening it.
|
||||
let restricted = FileSystemEnvironmentAccessor::new(
|
||||
&file_system,
|
||||
FileSystemSandboxContext::from_permission_profile(
|
||||
narrowed_permissions.clone(),
|
||||
executor_cwd.clone(),
|
||||
),
|
||||
);
|
||||
test.submit_turn_with_permission_profile(
|
||||
"Use $repo-revoked if it is still available.",
|
||||
narrowed_permissions.clone(),
|
||||
)
|
||||
.await?;
|
||||
snapshot
|
||||
.read_skill_text(&revoked_skill, &[&restricted])
|
||||
.await
|
||||
.expect_err("old metadata cannot reopen a now-denied skill");
|
||||
assert!(
|
||||
snapshot
|
||||
.read_skill_text(&revoked_skill, &[&open])
|
||||
.await?
|
||||
.contains(GATED_BODY)
|
||||
);
|
||||
|
||||
// An allowed skill path must not bypass the denial of its new symlink target.
|
||||
let (link, target) = match change {
|
||||
CachedRepositorySkillChange::ReplaceFileWithSymlink => {
|
||||
(gated_path, denied_directory.join("SKILL.md")?)
|
||||
}
|
||||
CachedRepositorySkillChange::ReplaceDirectoryWithSymlink => (
|
||||
gated_path.parent().expect("skill directory"),
|
||||
denied_directory,
|
||||
),
|
||||
};
|
||||
file_system
|
||||
.remove(
|
||||
&link,
|
||||
RemoveOptions {
|
||||
recursive: true,
|
||||
force: false,
|
||||
follow_symlinks: true,
|
||||
},
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await?;
|
||||
let process = test
|
||||
.executor_environment()
|
||||
.environment()
|
||||
.get_exec_backend()
|
||||
.start(ExecParams {
|
||||
process_id: ProcessId::from("replace-cached-skill"),
|
||||
metadata: None,
|
||||
argv: vec![
|
||||
"/bin/ln".to_string(),
|
||||
"-s".to_string(),
|
||||
target.inferred_native_path_string(),
|
||||
link.inferred_native_path_string(),
|
||||
],
|
||||
cwd: executor_cwd.clone(),
|
||||
env_policy: None,
|
||||
shell_snapshot: None,
|
||||
env: Default::default(),
|
||||
tty: false,
|
||||
pipe_stdin: false,
|
||||
arg0: None,
|
||||
sandbox: None,
|
||||
enforce_managed_network: false,
|
||||
managed_network: None,
|
||||
network_proxy: None,
|
||||
})
|
||||
.await?
|
||||
.process;
|
||||
let mut events = process.subscribe_events();
|
||||
loop {
|
||||
match events.recv().await? {
|
||||
ExecProcessEvent::Exited { exit_code, .. } => {
|
||||
assert_eq!(exit_code, 0, "replace the cached skill on its executor");
|
||||
break;
|
||||
}
|
||||
ExecProcessEvent::Output(_) => {}
|
||||
event @ (ExecProcessEvent::Closed { .. } | ExecProcessEvent::Failed(_)) => {
|
||||
panic!("skill symlink process did not exit: {event:?}")
|
||||
}
|
||||
}
|
||||
}
|
||||
test.submit_turn_with_permission_profile(
|
||||
"Use $repo-gated if it is still available.",
|
||||
narrowed_permissions,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let requests = responses.requests();
|
||||
assert_eq!(requests.len(), 3);
|
||||
let first_developer_texts = requests[0].message_input_texts("developer");
|
||||
let first_catalog = catalog_text(&first_developer_texts, "repo");
|
||||
assert!(first_catalog.contains("- repo-revoked:"));
|
||||
|
||||
let second_developer_texts = requests[1].message_input_texts("developer");
|
||||
assert!(
|
||||
second_developer_texts
|
||||
.iter()
|
||||
.any(|text| text == first_catalog)
|
||||
);
|
||||
for request in &requests[1..] {
|
||||
assert!(
|
||||
request
|
||||
.message_input_texts("user")
|
||||
.iter()
|
||||
.all(|text| !text.contains(GATED_BODY))
|
||||
);
|
||||
}
|
||||
|
||||
snapshot
|
||||
.read_skill_text(&gated_skill, &[&restricted])
|
||||
.await
|
||||
.expect_err("old metadata cannot reopen a now-denied skill");
|
||||
let restored = snapshot.read_skill_text(&gated_skill, &[&open]).await?;
|
||||
assert!(restored.contains(GATED_BODY));
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn agent_plugin_skill_prompt_stays_bounded_without_skills_extension() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -541,7 +541,14 @@ impl<C> SkillsExtension<C> {
|
||||
let context = match entry.authority.kind {
|
||||
SkillSourceKind::Cloud => SkillReadContext::Cloud { mcp_resources },
|
||||
SkillSourceKind::Custom(_) => SkillReadContext::Custom { mcp_resources },
|
||||
SkillSourceKind::Host => SkillReadContext::Host { host_snapshot },
|
||||
SkillSourceKind::Host => SkillReadContext::Host {
|
||||
host_snapshot,
|
||||
accessors: input
|
||||
.environments
|
||||
.iter()
|
||||
.map(|environment| environment.fs)
|
||||
.collect(),
|
||||
},
|
||||
SkillSourceKind::Executor => {
|
||||
let fs = entry
|
||||
.main_prompt
|
||||
|
||||
@@ -4,9 +4,13 @@ use std::fmt;
|
||||
use std::io;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_exec_server::EnvironmentAccessExt;
|
||||
use codex_exec_server::EnvironmentAccessKey;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_exec_server::ReadFileOptions;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_skills::SkillError;
|
||||
use codex_skills::SkillMetadata;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
@@ -35,7 +39,7 @@ impl SkillLoadOutcome {
|
||||
skill_root_by_path: HashMap<PathUri, AbsolutePathBuf>,
|
||||
skill_discovery_path_by_path: HashMap<PathUri, PathUri>,
|
||||
agent_plugin_skill_paths: HashSet<PathUri>,
|
||||
file_systems_by_skill_path: HashMap<PathUri, Arc<dyn ExecutorFileSystem>>,
|
||||
file_systems_by_skill_path: HashMap<PathUri, EnvironmentAccessKey>,
|
||||
) -> Self {
|
||||
Self {
|
||||
skills,
|
||||
@@ -108,22 +112,38 @@ impl SkillLoadOutcome {
|
||||
pub(crate) fn file_system_for_skill(
|
||||
&self,
|
||||
skill: &SkillMetadata,
|
||||
) -> Option<Arc<dyn ExecutorFileSystem>> {
|
||||
) -> Option<&EnvironmentAccessKey> {
|
||||
self.file_systems_by_skill_path
|
||||
.get(&skill.path_to_skills_md)
|
||||
}
|
||||
|
||||
/// Reads one loaded skill through the filesystem that discovered it.
|
||||
pub(crate) async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result<String> {
|
||||
let fs = self
|
||||
.file_system_for_skill(skill)
|
||||
.unwrap_or_else(|| Arc::clone(&LOCAL_FS));
|
||||
fs.read_file_text(
|
||||
&skill.path_to_skills_md,
|
||||
ReadFileOptions::default(),
|
||||
/*sandbox*/ None,
|
||||
)
|
||||
.await
|
||||
/// Reads through current access to the original filesystem; cached metadata grants no access.
|
||||
pub(crate) async fn read_skill_text(
|
||||
&self,
|
||||
skill: &SkillMetadata,
|
||||
accessors: &[&dyn EnvironmentAccess],
|
||||
) -> io::Result<String> {
|
||||
let host = FileSystemEnvironmentAccessor::unrestricted(&LOCAL_FS);
|
||||
let access: &dyn EnvironmentAccess = match skill.scope {
|
||||
SkillScope::Repo => self
|
||||
.file_system_for_skill(skill)
|
||||
.and_then(|source| {
|
||||
accessors
|
||||
.iter()
|
||||
.copied()
|
||||
.find(|access| source.same_file_system(&access.cache_key()))
|
||||
})
|
||||
.ok_or_else(|| {
|
||||
io::Error::new(
|
||||
io::ErrorKind::PermissionDenied,
|
||||
"skill discovery filesystem is unavailable for this callback",
|
||||
)
|
||||
})?,
|
||||
SkillScope::User | SkillScope::System | SkillScope::Admin => &host,
|
||||
};
|
||||
access
|
||||
.read_file_text(&skill.path_to_skills_md, ReadFileOptions::default())
|
||||
.await
|
||||
}
|
||||
}
|
||||
|
||||
@@ -159,18 +179,18 @@ impl codex_skills::ExplicitSkillLookup for SkillLoadOutcome {
|
||||
|
||||
#[derive(Clone, Default)]
|
||||
pub(crate) struct SkillFileSystemsByPath {
|
||||
values: Arc<HashMap<PathUri, Arc<dyn ExecutorFileSystem>>>,
|
||||
values: Arc<HashMap<PathUri, EnvironmentAccessKey>>,
|
||||
}
|
||||
|
||||
impl SkillFileSystemsByPath {
|
||||
pub(crate) fn new(values: HashMap<PathUri, Arc<dyn ExecutorFileSystem>>) -> Self {
|
||||
pub(crate) fn new(values: HashMap<PathUri, EnvironmentAccessKey>) -> Self {
|
||||
Self {
|
||||
values: Arc::new(values),
|
||||
}
|
||||
}
|
||||
|
||||
fn get(&self, path: &PathUri) -> Option<Arc<dyn ExecutorFileSystem>> {
|
||||
self.values.get(path).map(Arc::clone)
|
||||
fn get(&self, path: &PathUri) -> Option<&EnvironmentAccessKey> {
|
||||
self.values.get(path)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use std::collections::HashSet;
|
||||
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_extension_api::ContextualUserFragment;
|
||||
use codex_skills::SkillMetadata;
|
||||
use codex_skills::normalize_skill_path;
|
||||
@@ -66,7 +67,11 @@ impl HostSkillsSnapshot {
|
||||
skip_all,
|
||||
fields(selected_skill_count = selected_skills.len())
|
||||
)]
|
||||
pub async fn load_skill_prompts(&self, selected_skills: &[SkillMetadata]) -> HostSkillPrompts {
|
||||
pub async fn load_skill_prompts(
|
||||
&self,
|
||||
selected_skills: &[SkillMetadata],
|
||||
accessors: &[&dyn EnvironmentAccess],
|
||||
) -> HostSkillPrompts {
|
||||
let mut prompts = HostSkillPrompts {
|
||||
fragments: Vec::with_capacity(selected_skills.len()),
|
||||
injected: Vec::with_capacity(selected_skills.len()),
|
||||
@@ -74,7 +79,7 @@ impl HostSkillsSnapshot {
|
||||
};
|
||||
|
||||
for skill in selected_skills {
|
||||
match self.read_skill_text(skill).await {
|
||||
match self.read_skill_text(skill, accessors).await {
|
||||
Ok(contents) => {
|
||||
let (contents, truncated) = if self.outcome().is_agent_plugin_skill(skill) {
|
||||
truncate_main_prompt_contents(&contents)
|
||||
|
||||
@@ -2,6 +2,7 @@ use std::io;
|
||||
use std::sync::Arc;
|
||||
|
||||
use crate::SkillLoadOutcome;
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_skills::SkillMetadata;
|
||||
|
||||
/// Immutable snapshot of host-owned skills and their source filesystems.
|
||||
@@ -19,7 +20,11 @@ impl HostSkillsSnapshot {
|
||||
self.outcome.as_ref()
|
||||
}
|
||||
|
||||
pub async fn read_skill_text(&self, skill: &SkillMetadata) -> io::Result<String> {
|
||||
self.outcome.read_skill_text(skill).await
|
||||
pub async fn read_skill_text(
|
||||
&self,
|
||||
skill: &SkillMetadata,
|
||||
accessors: &[&dyn EnvironmentAccess],
|
||||
) -> io::Result<String> {
|
||||
self.outcome.read_skill_text(skill, accessors).await
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_exec_server::EnvironmentAccessKey;
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_exec_server::GetMetadataOptions;
|
||||
@@ -117,7 +119,7 @@ pub(crate) struct HostSkillRootSnapshot {
|
||||
pub(crate) skills: Vec<SkillMetadata>,
|
||||
pub(crate) skill_discovery_path_by_path: Arc<HashMap<PathUri, PathUri>>,
|
||||
pub(crate) errors: Vec<SkillError>,
|
||||
pub(crate) file_system: Arc<dyn ExecutorFileSystem>,
|
||||
pub(crate) file_system: EnvironmentAccessKey,
|
||||
pub(crate) is_agent_plugin: bool,
|
||||
}
|
||||
|
||||
@@ -138,7 +140,7 @@ pub(crate) async fn load_host_skill_root(root: HostSkillRoot) -> HostSkillRootSn
|
||||
skills,
|
||||
skill_discovery_path_by_path,
|
||||
errors,
|
||||
file_system: root.file_system,
|
||||
file_system: FileSystemEnvironmentAccessor::unrestricted(&root.file_system).cache_key(),
|
||||
is_agent_plugin,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -2,7 +2,9 @@ use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
use std::sync::Arc;
|
||||
|
||||
use codex_exec_server::ExecutorFileSystem;
|
||||
use codex_exec_server::EnvironmentAccess;
|
||||
use codex_exec_server::EnvironmentAccessKey;
|
||||
use codex_exec_server::FileSystemEnvironmentAccessor;
|
||||
use codex_protocol::protocol::Product;
|
||||
use codex_protocol::protocol::SkillScope;
|
||||
use codex_skills::LoadedSkillRoot;
|
||||
@@ -88,7 +90,8 @@ pub(crate) async fn load_and_merge_host_skill_roots_with_request_snapshots(
|
||||
skills: snapshot.skills,
|
||||
skill_discovery_path_by_path: snapshot.skill_discovery_path_by_path,
|
||||
errors: snapshot.errors,
|
||||
file_system: Arc::clone(&file_system),
|
||||
file_system: FileSystemEnvironmentAccessor::unrestricted(&file_system)
|
||||
.cache_key(),
|
||||
is_agent_plugin: snapshot.is_agent_plugin,
|
||||
},
|
||||
None => {
|
||||
@@ -202,7 +205,7 @@ fn merge_host_skill_root_snapshots(snapshots: Vec<HostSkillRootSnapshot>) -> Ski
|
||||
let mut skill_root_by_path = HashMap::new();
|
||||
let mut skill_discovery_path_by_path = HashMap::new();
|
||||
let mut agent_plugin_skill_paths = HashSet::new();
|
||||
let mut file_systems_by_skill_path = HashMap::<PathUri, Arc<dyn ExecutorFileSystem>>::new();
|
||||
let mut file_systems_by_skill_path = HashMap::<PathUri, EnvironmentAccessKey>::new();
|
||||
|
||||
for snapshot in snapshots {
|
||||
if !snapshot.skills.is_empty() && !skill_roots.contains(&snapshot.root) {
|
||||
@@ -215,7 +218,7 @@ fn merge_host_skill_root_snapshots(snapshots: Vec<HostSkillRootSnapshot>) -> Ski
|
||||
if let Some(discovery_path) = snapshot.skill_discovery_path_by_path.get(&path) {
|
||||
skill_discovery_path_by_path.insert(path.clone(), discovery_path.clone());
|
||||
}
|
||||
file_systems_by_skill_path.insert(path.clone(), Arc::clone(&snapshot.file_system));
|
||||
file_systems_by_skill_path.insert(path.clone(), snapshot.file_system.clone());
|
||||
if snapshot.is_agent_plugin {
|
||||
agent_plugin_skill_paths.insert(path);
|
||||
}
|
||||
|
||||
@@ -54,11 +54,12 @@ pub struct SkillReadRequest<'a> {
|
||||
pub context: SkillReadContext<'a>,
|
||||
}
|
||||
|
||||
/// Executor reads require callback access; other providers carry their own source context.
|
||||
/// Repository and executor reads require callback access; other sources retain provider context.
|
||||
#[derive(Clone)]
|
||||
pub enum SkillReadContext<'a> {
|
||||
Host {
|
||||
host_snapshot: Option<Arc<HostSkillsSnapshot>>,
|
||||
accessors: Vec<&'a dyn EnvironmentAccess>,
|
||||
},
|
||||
Executor {
|
||||
fs: &'a dyn EnvironmentAccess,
|
||||
|
||||
@@ -55,6 +55,7 @@ impl SkillProvider for HostSkillProvider {
|
||||
Box::pin(async move {
|
||||
let SkillReadContext::Host {
|
||||
host_snapshot: Some(host_snapshot),
|
||||
accessors,
|
||||
} = request.context
|
||||
else {
|
||||
return Err(SkillProviderError::new(
|
||||
@@ -78,12 +79,15 @@ impl SkillProvider for HostSkillProvider {
|
||||
)));
|
||||
};
|
||||
|
||||
let contents = host_snapshot.read_skill_text(skill).await.map_err(|err| {
|
||||
SkillProviderError::new(format!(
|
||||
"failed to read host skill resource {}: {err}",
|
||||
request.resource.as_str()
|
||||
))
|
||||
})?;
|
||||
let contents = host_snapshot
|
||||
.read_skill_text(skill, &accessors)
|
||||
.await
|
||||
.map_err(|err| {
|
||||
SkillProviderError::new(format!(
|
||||
"failed to read host skill resource {}: {err}",
|
||||
request.resource.as_str()
|
||||
))
|
||||
})?;
|
||||
|
||||
Ok(SkillReadResult {
|
||||
resource: request.resource,
|
||||
|
||||
@@ -110,6 +110,7 @@ async fn host_catalog_resource_spellings_read_the_discovered_skill()
|
||||
package: entry.id.clone(),
|
||||
resource: resource.clone(),
|
||||
context: SkillReadContext::Host {
|
||||
accessors: Vec::new(),
|
||||
host_snapshot: Some(Arc::clone(&snapshot)),
|
||||
},
|
||||
})
|
||||
|
||||
@@ -287,14 +287,16 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() {
|
||||
/*bundled_skills_enabled*/ false,
|
||||
/*restriction_product*/ None,
|
||||
);
|
||||
let file_system: Arc<dyn ExecutorFileSystem> = Arc::new(SyntheticFileSystem {
|
||||
alias_root: PathUri::from_abs_path(&alias_root),
|
||||
canonical_root: PathUri::from_abs_path(&canonical_root),
|
||||
has_plugin_manifest: false,
|
||||
});
|
||||
let access = FileSystemEnvironmentAccessor::unrestricted(&file_system);
|
||||
let snapshot = service
|
||||
.snapshot_for_config(
|
||||
&HostSkillsLoadInput::new(cwd, Vec::new(), config_layer_stack),
|
||||
Some(Arc::new(SyntheticFileSystem {
|
||||
alias_root: PathUri::from_abs_path(&alias_root),
|
||||
canonical_root: PathUri::from_abs_path(&canonical_root),
|
||||
has_plugin_manifest: false,
|
||||
})),
|
||||
Some(Arc::clone(&file_system)),
|
||||
)
|
||||
.await;
|
||||
let outcome = snapshot.outcome();
|
||||
@@ -308,9 +310,41 @@ async fn skill_loading_and_reads_use_the_supplied_executor_file_system() {
|
||||
PathUri::from_abs_path(&canonical_root.join("skill/SKILL.md"))
|
||||
);
|
||||
assert_eq!(
|
||||
snapshot.read_skill_text(&skill).await.expect("skill body"),
|
||||
snapshot
|
||||
.read_skill_text(&skill, &[&access])
|
||||
.await
|
||||
.expect("skill body"),
|
||||
SKILL_CONTENTS
|
||||
);
|
||||
assert_eq!(
|
||||
snapshot
|
||||
.read_skill_text(&skill, &[])
|
||||
.await
|
||||
.unwrap_err()
|
||||
.kind(),
|
||||
io::ErrorKind::PermissionDenied,
|
||||
);
|
||||
let replacement: Arc<dyn ExecutorFileSystem> = Arc::new(SyntheticFileSystem {
|
||||
alias_root: PathUri::from_abs_path(&alias_root),
|
||||
canonical_root: PathUri::from_abs_path(&canonical_root),
|
||||
has_plugin_manifest: false,
|
||||
});
|
||||
let replacement = FileSystemEnvironmentAccessor::unrestricted(&replacement);
|
||||
assert_eq!(
|
||||
snapshot
|
||||
.read_skill_text(&skill, &[&replacement])
|
||||
.await
|
||||
.unwrap_err()
|
||||
.kind(),
|
||||
io::ErrorKind::PermissionDenied,
|
||||
);
|
||||
assert_eq!(
|
||||
snapshot
|
||||
.read_skill_text(&skill, &[&replacement, &access])
|
||||
.await
|
||||
.expect("select the original filesystem regardless of accessor order"),
|
||||
SKILL_CONTENTS,
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -130,6 +130,13 @@ pub struct EnvironmentAccessKey {
|
||||
sandbox: Option<Arc<FileSystemSandboxContext>>,
|
||||
}
|
||||
|
||||
impl EnvironmentAccessKey {
|
||||
/// Compares filesystem identity without comparing captured permissions.
|
||||
pub fn same_file_system(&self, other: &Self) -> bool {
|
||||
self.file_system.ptr_eq(&other.file_system)
|
||||
}
|
||||
}
|
||||
|
||||
impl fmt::Debug for EnvironmentAccessKey {
|
||||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
|
||||
f.write_str("EnvironmentAccessKey(..)")
|
||||
|
||||
Reference in New Issue
Block a user