From 150cb18230e95bfbff37c56d4f65bcad5e41dccd Mon Sep 17 00:00:00 2001 From: starr-openai Date: Thu, 16 Apr 2026 17:28:55 -0700 Subject: [PATCH] Explore executor-bound path API Adds executor-bound path helpers and threads them through skills, config loading, AGENTS.md loading, view_image, and apply_patch to evaluate the API blast radius. Harness-owned reads use explicit unsandboxed access while tool-call paths carry the sandbox context through the path access wrapper. Validation: just fmt; git diff --check. Co-authored-by: Codex --- .../app-server/src/codex_message_processor.rs | 96 +++-- codex-rs/apply-patch/src/invocation.rs | 19 +- codex-rs/apply-patch/src/lib.rs | 157 +++---- codex-rs/core-plugins/src/loader.rs | 4 +- codex-rs/core-skills/src/injection.rs | 15 +- codex-rs/core-skills/src/loader.rs | 196 ++++----- codex-rs/core-skills/src/loader_tests.rs | 54 +-- codex-rs/core-skills/src/manager.rs | 45 +- codex-rs/core-skills/src/model.rs | 34 +- codex-rs/core/src/agents_md.rs | 52 +-- codex-rs/core/src/codex/handlers.rs | 21 +- codex-rs/core/src/codex/turn.rs | 5 + codex-rs/core/src/config/agent_roles.rs | 64 +-- codex-rs/core/src/config/mod.rs | 22 +- codex-rs/core/src/config_loader/README.md | 2 + codex-rs/core/src/config_loader/layer_io.rs | 30 +- codex-rs/core/src/config_loader/mod.rs | 195 +++++---- codex-rs/core/src/config_loader/tests.rs | 18 +- .../core/src/tools/handlers/view_image.rs | 45 +- codex-rs/exec-server/src/file_system.rs | 396 ++++++++++++++++++ codex-rs/exec-server/src/lib.rs | 3 + codex-rs/git-utils/src/info.rs | 41 +- codex-rs/git-utils/src/lib.rs | 1 + .../utils/plugins/src/plugin_namespace.rs | 34 +- 24 files changed, 1011 insertions(+), 538 deletions(-) diff --git a/codex-rs/app-server/src/codex_message_processor.rs b/codex-rs/app-server/src/codex_message_processor.rs index 6a9dfb4042..5dfe836a62 100644 --- a/codex-rs/app-server/src/codex_message_processor.rs +++ b/codex-rs/app-server/src/codex_message_processor.rs @@ -221,8 +221,9 @@ use codex_core::config::edit::ConfigEditsBuilder; use codex_core::config_loader::CloudRequirementsLoadError; use codex_core::config_loader::CloudRequirementsLoadErrorCode; use codex_core::config_loader::CloudRequirementsLoader; +use codex_core::config_loader::ConfigLoadFileSystems; use codex_core::config_loader::LoaderOverrides; -use codex_core::config_loader::load_config_layers_state; +use codex_core::config_loader::load_config_layers_state_with_file_systems; use codex_core::config_loader::project_trust_key; use codex_core::exec::ExecCapturePolicy; use codex_core::exec::ExecExpiration; @@ -251,6 +252,7 @@ use codex_core_plugins::loader::load_plugin_mcp_servers; use codex_core_plugins::manifest::PluginManifestInterface; use codex_core_plugins::marketplace::MarketplaceError; use codex_core_plugins::marketplace::MarketplacePluginSource; +use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; use codex_features::FEATURES; use codex_features::Feature; @@ -621,9 +623,25 @@ pub(crate) struct CodexMessageProcessorArgs { } impl CodexMessageProcessor { - async fn instruction_sources_from_config(config: &Config) -> Vec { + async fn thread_filesystem_or_local( + thread_manager: &ThreadManager, + ) -> Arc { + match thread_manager.environment_manager().current().await { + Ok(Some(environment)) => environment.get_filesystem(), + Ok(None) => Arc::clone(&LOCAL_FS), + Err(err) => { + warn!("failed to get current environment filesystem: {err}"); + Arc::clone(&LOCAL_FS) + } + } + } + + async fn instruction_sources_from_config( + config: &Config, + fs: Arc, + ) -> Vec { codex_core::AgentsMdManager::new(config) - .instruction_sources(LOCAL_FS.as_ref()) + .instruction_sources(fs.as_ref()) .await } @@ -2417,6 +2435,8 @@ impl CodexMessageProcessor { return; } }; + let thread_fs = + Self::thread_filesystem_or_local(&listener_task_context.thread_manager).await; // The user may have requested WorkspaceWrite or DangerFullAccess via // the command line, though in the process of deriving the Config, it @@ -2441,7 +2461,7 @@ impl CodexMessageProcessor { | codex_protocol::protocol::SandboxPolicy::ExternalSandbox { .. } )) { - let trust_target = resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &config.cwd) + let trust_target = resolve_root_git_project_for_trust(thread_fs.as_ref(), &config.cwd) .await .unwrap_or_else(|| config.cwd.clone()); let cli_overrides_with_trust; @@ -2500,7 +2520,8 @@ impl CodexMessageProcessor { }; } - let instruction_sources = Self::instruction_sources_from_config(&config).await; + let instruction_sources = + Self::instruction_sources_from_config(&config, thread_fs.clone()).await; let dynamic_tools = dynamic_tools.unwrap_or_default(); let core_dynamic_tools = if dynamic_tools.is_empty() { Vec::new() @@ -4092,7 +4113,9 @@ impl CodexMessageProcessor { }; let fallback_model_provider = config.model_provider_id.clone(); - let instruction_sources = Self::instruction_sources_from_config(&config).await; + let instruction_sources_fs = Self::thread_filesystem_or_local(&self.thread_manager).await; + let instruction_sources = + Self::instruction_sources_from_config(&config, instruction_sources_fs).await; let response_history = thread_history.clone(); match self @@ -4357,8 +4380,13 @@ impl CodexMessageProcessor { } let mut config_for_instruction_sources = self.config.as_ref().clone(); config_for_instruction_sources.cwd = config_snapshot.cwd.clone(); - let instruction_sources = - Self::instruction_sources_from_config(&config_for_instruction_sources).await; + let instruction_sources_fs = + Self::thread_filesystem_or_local(&self.thread_manager).await; + let instruction_sources = Self::instruction_sources_from_config( + &config_for_instruction_sources, + instruction_sources_fs, + ) + .await; let thread_summary = match load_thread_summary_for_rollout( &self.config, existing_thread_id, @@ -4672,7 +4700,9 @@ impl CodexMessageProcessor { }; let fallback_model_provider = config.model_provider_id.clone(); - let instruction_sources = Self::instruction_sources_from_config(&config).await; + let instruction_sources_fs = Self::thread_filesystem_or_local(&self.thread_manager).await; + let instruction_sources = + Self::instruction_sources_from_config(&config, instruction_sources_fs).await; let NewThread { thread_id, @@ -6116,30 +6146,36 @@ impl CodexMessageProcessor { }; let skills_manager = self.thread_manager.skills_manager(); let plugins_manager = self.thread_manager.plugins_manager(); - let fs = match self.thread_manager.environment_manager().current().await { - Ok(Some(environment)) => Some(environment.get_filesystem()), - Ok(None) => None, - Err(err) => { - self.outgoing - .send_error( - request_id, - JSONRPCErrorError { - code: INTERNAL_ERROR_CODE, - message: format!("failed to create environment: {err}"), - data: None, - }, - ) - .await; - return; - } - }; + let (fs, remote_project_fs) = + match self.thread_manager.environment_manager().current().await { + Ok(Some(environment)) => (environment.get_filesystem(), environment.is_remote()), + Ok(None) => (Arc::clone(&LOCAL_FS), false), + Err(err) => { + self.outgoing + .send_error( + request_id, + JSONRPCErrorError { + code: INTERNAL_ERROR_CODE, + message: format!("failed to create environment: {err}"), + data: None, + }, + ) + .await; + return; + } + }; let cli_overrides = self.current_cli_overrides(); let mut data = Vec::new(); for cwd in cwds { let extra_roots = extra_roots_by_cwd .get(&cwd) .map_or(&[][..], std::vec::Vec::as_slice); - let cwd_abs = match AbsolutePathBuf::relative_to_current_dir(cwd.as_path()) { + let cwd_abs = if remote_project_fs { + AbsolutePathBuf::from_absolute_path_checked(cwd.as_path()) + } else { + AbsolutePathBuf::relative_to_current_dir(cwd.as_path()) + }; + let cwd_abs = match cwd_abs { Ok(path) => path, Err(err) => { let error_path = cwd.clone(); @@ -6154,8 +6190,8 @@ impl CodexMessageProcessor { continue; } }; - let config_layer_stack = match load_config_layers_state( - LOCAL_FS.as_ref(), + let config_layer_stack = match load_config_layers_state_with_file_systems( + ConfigLoadFileSystems::same(fs.as_ref()), &self.config.codex_home, Some(cwd_abs.clone()), &cli_overrides, @@ -6195,7 +6231,7 @@ impl CodexMessageProcessor { &skills_input, force_reload, extra_roots, - fs.clone(), + Some(Arc::clone(&fs)), ) .await; let errors = errors_to_info(&outcome.errors); diff --git a/codex-rs/apply-patch/src/invocation.rs b/codex-rs/apply-patch/src/invocation.rs index 075c94c60c..f36f117ca0 100644 --- a/codex-rs/apply-patch/src/invocation.rs +++ b/codex-rs/apply-patch/src/invocation.rs @@ -3,6 +3,7 @@ use std::path::Path; use std::sync::LazyLock; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPathRef; use codex_utils_absolute_path::AbsolutePathBuf; use tree_sitter::Parser; use tree_sitter::Query; @@ -20,7 +21,7 @@ use crate::MaybeApplyPatchVerified; use crate::parser::Hunk; use crate::parser::ParseError; use crate::parser::parse_patch; -use crate::unified_diff_from_chunks; +use crate::unified_diff_from_chunks_at; use std::str::Utf8Error; use tree_sitter::LanguageError; @@ -162,16 +163,16 @@ pub async fn maybe_parse_apply_patch_verified( .unwrap_or_else(|| cwd.clone()); let mut changes = HashMap::new(); for hunk in hunks { - let path = hunk.resolve_path(&effective_cwd); + let path = ExecutorPathRef::new(fs, hunk.resolve_path(&effective_cwd)); match hunk { Hunk::AddFile { contents, .. } => { changes.insert( - path.into_path_buf(), + path.to_path_buf(), ApplyPatchFileChange::Add { content: contents }, ); } Hunk::DeleteFile { .. } => { - let content = match fs.read_file_text(&path, sandbox).await { + let content = match path.with_sandbox(sandbox).read_file_text().await { Ok(content) => content, Err(e) => { return MaybeApplyPatchVerified::CorrectnessError( @@ -182,10 +183,8 @@ pub async fn maybe_parse_apply_patch_verified( ); } }; - changes.insert( - path.into_path_buf(), - ApplyPatchFileChange::Delete { content }, - ); + changes + .insert(path.to_path_buf(), ApplyPatchFileChange::Delete { content }); } Hunk::UpdateFile { move_path, chunks, .. @@ -193,14 +192,14 @@ pub async fn maybe_parse_apply_patch_verified( let ApplyPatchFileUpdate { unified_diff, content: contents, - } = match unified_diff_from_chunks(&path, &chunks, fs, sandbox).await { + } = match unified_diff_from_chunks_at(&path, &chunks, sandbox).await { Ok(diff) => diff, Err(e) => { return MaybeApplyPatchVerified::CorrectnessError(e); } }; changes.insert( - path.into_path_buf(), + path.to_path_buf(), ApplyPatchFileChange::Update { unified_diff, move_path: move_path.map(|p| effective_cwd.join(p).into_path_buf()), diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 34fb4e95c1..c7f7164129 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -12,6 +12,7 @@ use anyhow::Context; use anyhow::Result; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPathRef; use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::RemoveOptions; use codex_utils_absolute_path::AbsolutePathBuf; @@ -224,7 +225,8 @@ pub async fn apply_hunks( sandbox: Option<&FileSystemSandboxContext>, ) -> Result<(), ApplyPatchError> { // Delegate to a helper that applies each hunk to the filesystem. - match apply_hunks_to_files(hunks, cwd, fs, sandbox).await { + let cwd = ExecutorPathRef::new(fs, cwd.clone()); + match apply_hunks_to_files(hunks, &cwd, sandbox).await { Ok(affected) => { print_summary(&affected, stdout).map_err(ApplyPatchError::from)?; Ok(()) @@ -258,8 +260,7 @@ pub struct AffectedPaths { /// Returns an error if the patch could not be applied. async fn apply_hunks_to_files( hunks: &[Hunk], - cwd: &AbsolutePathBuf, - fs: &dyn ExecutorFileSystem, + cwd: &ExecutorPathRef<'_>, sandbox: Option<&FileSystemSandboxContext>, ) -> anyhow::Result { if hunks.is_empty() { @@ -271,82 +272,71 @@ async fn apply_hunks_to_files( let mut deleted: Vec = Vec::new(); for hunk in hunks { let affected_path = hunk.path().to_path_buf(); - let path_abs = hunk.resolve_path(cwd); + let path = cwd.with_path(hunk.resolve_path(cwd.path())); match hunk { Hunk::AddFile { contents, .. } => { - write_file_with_missing_parent_retry( - fs, - &path_abs, - contents.clone().into_bytes(), - sandbox, - ) - .await?; + write_file_with_missing_parent_retry(&path, contents.clone().into_bytes(), sandbox) + .await?; added.push(affected_path); } Hunk::DeleteFile { .. } => { let result: io::Result<()> = async { - let metadata = fs.get_metadata(&path_abs, sandbox).await?; + let path_access = path.with_sandbox(sandbox); + let metadata = path_access.get_metadata().await?; if metadata.is_directory { return Err(io::Error::new( io::ErrorKind::InvalidInput, "path is a directory", )); } - fs.remove( - &path_abs, - RemoveOptions { + path_access + .remove(RemoveOptions { recursive: false, force: false, - }, - sandbox, - ) - .await + }) + .await } .await; - result.with_context(|| format!("Failed to delete file {}", path_abs.display()))?; + result.with_context(|| format!("Failed to delete file {}", path.display()))?; deleted.push(affected_path); } Hunk::UpdateFile { move_path, chunks, .. } => { let AppliedPatch { new_contents, .. } = - derive_new_contents_from_chunks(&path_abs, chunks, fs, sandbox).await?; + derive_new_contents_from_chunks(&path, chunks, sandbox).await?; if let Some(dest) = move_path { - let dest_abs = AbsolutePathBuf::resolve_path_against_base(dest, cwd); - write_file_with_missing_parent_retry( - fs, - &dest_abs, - new_contents.into_bytes(), - sandbox, - ) - .await?; + let dest = cwd.with_path(AbsolutePathBuf::resolve_path_against_base( + dest, + cwd.path().as_path(), + )); + write_file_with_missing_parent_retry(&dest, new_contents.into_bytes(), sandbox) + .await?; let result: io::Result<()> = async { - let metadata = fs.get_metadata(&path_abs, sandbox).await?; + let path_access = path.with_sandbox(sandbox); + let metadata = path_access.get_metadata().await?; if metadata.is_directory { return Err(io::Error::new( io::ErrorKind::InvalidInput, "path is a directory", )); } - fs.remove( - &path_abs, - RemoveOptions { + path_access + .remove(RemoveOptions { recursive: false, force: false, - }, - sandbox, - ) - .await + }) + .await } .await; - result.with_context(|| { - format!("Failed to remove original {}", path_abs.display()) - })?; + result + .with_context(|| format!("Failed to remove original {}", path.display()))?; modified.push(affected_path); } else { - fs.write_file(&path_abs, new_contents.into_bytes(), sandbox) + path.with_sandbox(sandbox) + .write_file(new_contents.into_bytes()) .await - .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + .with_context(|| format!("Failed to write file {}", path.display()))?; modified.push(affected_path); } } @@ -360,36 +350,33 @@ async fn apply_hunks_to_files( } async fn write_file_with_missing_parent_retry( - fs: &dyn ExecutorFileSystem, - path_abs: &AbsolutePathBuf, + path: &ExecutorPathRef<'_>, contents: Vec, sandbox: Option<&FileSystemSandboxContext>, ) -> anyhow::Result<()> { - match fs.write_file(path_abs, contents.clone(), sandbox).await { + match path + .with_sandbox(sandbox) + .write_file(contents.clone()) + .await + { Ok(()) => Ok(()), Err(err) if err.kind() == io::ErrorKind::NotFound => { - if let Some(parent_abs) = path_abs.parent() { - fs.create_directory( - &parent_abs, - CreateDirectoryOptions { recursive: true }, - sandbox, - ) - .await - .with_context(|| { - format!( - "Failed to create parent directories for {}", - path_abs.display() - ) - })?; + if let Some(parent) = path.parent() { + parent + .with_sandbox(sandbox) + .create_directory(CreateDirectoryOptions { recursive: true }) + .await + .with_context(|| { + format!("Failed to create parent directories for {}", path.display()) + })?; } - fs.write_file(path_abs, contents, sandbox) + path.with_sandbox(sandbox) + .write_file(contents) .await - .with_context(|| format!("Failed to write file {}", path_abs.display()))?; + .with_context(|| format!("Failed to write file {}", path.display()))?; Ok(()) } - Err(err) => { - Err(err).with_context(|| format!("Failed to write file {}", path_abs.display())) - } + Err(err) => Err(err).with_context(|| format!("Failed to write file {}", path.display())), } } @@ -401,17 +388,20 @@ struct AppliedPatch { /// Return *only* the new file contents (joined into a single `String`) after /// applying the chunks to the file at `path`. async fn derive_new_contents_from_chunks( - path_abs: &AbsolutePathBuf, + path: &ExecutorPathRef<'_>, chunks: &[UpdateFileChunk], - fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, ) -> std::result::Result { - let original_contents = fs.read_file_text(path_abs, sandbox).await.map_err(|err| { - ApplyPatchError::IoError(IoError { - context: format!("Failed to read file to update {}", path_abs.display()), - source: err, - }) - })?; + let original_contents = path + .with_sandbox(sandbox) + .read_file_text() + .await + .map_err(|err| { + ApplyPatchError::IoError(IoError { + context: format!("Failed to read file to update {}", path.display()), + source: err, + }) + })?; let mut original_lines: Vec = original_contents.split('\n').map(String::from).collect(); @@ -421,7 +411,7 @@ async fn derive_new_contents_from_chunks( original_lines.pop(); } - let replacements = compute_replacements(&original_lines, path_abs.as_path(), chunks)?; + let replacements = compute_replacements(&original_lines, path.path().as_path(), chunks)?; let new_lines = apply_replacements(original_lines, &replacements); let mut new_lines = new_lines; if !new_lines.last().is_some_and(String::is_empty) { @@ -568,7 +558,8 @@ pub async fn unified_diff_from_chunks( fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, ) -> std::result::Result { - unified_diff_from_chunks_with_context(path_abs, chunks, /*context*/ 1, fs, sandbox).await + let path = ExecutorPathRef::new(fs, path_abs.clone()); + unified_diff_from_chunks_at(&path, chunks, sandbox).await } pub async fn unified_diff_from_chunks_with_context( @@ -577,11 +568,29 @@ pub async fn unified_diff_from_chunks_with_context( context: usize, fs: &dyn ExecutorFileSystem, sandbox: Option<&FileSystemSandboxContext>, +) -> std::result::Result { + let path = ExecutorPathRef::new(fs, path_abs.clone()); + unified_diff_from_chunks_with_context_at(&path, chunks, context, sandbox).await +} + +pub async fn unified_diff_from_chunks_at( + path: &ExecutorPathRef<'_>, + chunks: &[UpdateFileChunk], + sandbox: Option<&FileSystemSandboxContext>, +) -> std::result::Result { + unified_diff_from_chunks_with_context_at(path, chunks, /*context*/ 1, sandbox).await +} + +pub async fn unified_diff_from_chunks_with_context_at( + path: &ExecutorPathRef<'_>, + chunks: &[UpdateFileChunk], + context: usize, + sandbox: Option<&FileSystemSandboxContext>, ) -> std::result::Result { let AppliedPatch { original_contents, new_contents, - } = derive_new_contents_from_chunks(path_abs, chunks, fs, sandbox).await?; + } = derive_new_contents_from_chunks(path, chunks, sandbox).await?; let text_diff = TextDiff::from_lines(&original_contents, &new_contents); let unified_diff = text_diff.unified_diff().context_radius(context).to_string(); Ok(ApplyPatchFileUpdate { diff --git a/codex-rs/core-plugins/src/loader.rs b/codex-rs/core-plugins/src/loader.rs index ff3b8fc494..d427f186bc 100644 --- a/codex-rs/core-plugins/src/loader.rs +++ b/codex-rs/core-plugins/src/loader.rs @@ -14,6 +14,7 @@ use codex_core_skills::config_rules::resolve_disabled_skill_paths; use codex_core_skills::config_rules::skill_config_rules_from_stack; use codex_core_skills::loader::SkillRoot; use codex_core_skills::loader::load_skills_from_roots; +use codex_exec_server::ExecutorPath; use codex_exec_server::LOCAL_FS; use codex_plugin::AppConnectorId; use codex_plugin::LoadedPlugin; @@ -532,9 +533,8 @@ pub async fn load_plugin_skills( let roots = plugin_skill_roots(plugin_root, manifest_paths) .into_iter() .map(|path| SkillRoot { - path, + path: ExecutorPath::new(Arc::clone(&LOCAL_FS), path), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), }) .collect::>(); let outcome = load_skills_from_roots(roots).await; diff --git a/codex-rs/core-skills/src/injection.rs b/codex-rs/core-skills/src/injection.rs index 1ccd447cf0..ba7b68f48d 100644 --- a/codex-rs/core-skills/src/injection.rs +++ b/codex-rs/core-skills/src/injection.rs @@ -9,7 +9,8 @@ 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::ExecutorFileSystem; +use codex_exec_server::ExecutorPath; use codex_instructions::SkillInstructions; use codex_otel::SessionTelemetry; use codex_protocol::models::ResponseItem; @@ -26,6 +27,7 @@ pub struct SkillInjections { pub async fn build_skill_injections( mentioned_skills: &[SkillMetadata], loaded_skills: Option<&SkillLoadOutcome>, + fs: Arc, otel: Option<&SessionTelemetry>, analytics_client: &AnalyticsEventsClient, tracking: TrackEventsContext, @@ -41,13 +43,10 @@ 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 - { + let source = loaded_skills + .and_then(|outcome| outcome.source_for_skill(skill)) + .unwrap_or_else(|| ExecutorPath::new(Arc::clone(&fs), skill.path_to_skills_md.clone())); + match source.unsandboxed().read_file_text().await { Ok(contents) => { emit_skill_injected_metric(otel, skill, "ok"); invocations.push(SkillInvocation { diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index 2cae6a4b0b..62b2e4d3df 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -1,10 +1,10 @@ use crate::model::SkillDependencies; use crate::model::SkillError; -use crate::model::SkillFileSystemsByPath; use crate::model::SkillInterface; use crate::model::SkillLoadOutcome; use crate::model::SkillMetadata; use crate::model::SkillPolicy; +use crate::model::SkillSourcesByPath; use crate::model::SkillToolDependency; use crate::system::system_cache_root_dir; use codex_app_server_protocol::ConfigLayerSource; @@ -14,6 +14,7 @@ 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::ExecutorFileSystem; +use codex_exec_server::ExecutorPath; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -149,9 +150,8 @@ impl fmt::Display for SkillParseError { impl Error for SkillParseError {} pub struct SkillRoot { - pub path: AbsolutePathBuf, + pub path: ExecutorPath, pub scope: SkillScope, - pub file_system: Arc, } pub async fn load_skills_from_roots(roots: I) -> SkillLoadOutcome @@ -159,16 +159,14 @@ where I: IntoIterator, { let mut outcome = SkillLoadOutcome::default(); - let mut file_systems_by_skill_path: HashMap> = - HashMap::new(); + let mut sources_by_skill_path: HashMap = HashMap::new(); for root in roots { - let fs = root.file_system; let skills_before_root = outcome.skills.len(); - discover_skills_under_root(fs.as_ref(), &root.path, root.scope, &mut outcome).await; + discover_skills_under_root(&root.path, root.scope, &mut outcome).await; for skill in &outcome.skills[skills_before_root..] { - file_systems_by_skill_path + sources_by_skill_path .entry(skill.path_to_skills_md.clone()) - .or_insert_with(|| Arc::clone(&fs)); + .or_insert_with(|| root.path.with_path(skill.path_to_skills_md.clone())); } } @@ -181,8 +179,8 @@ where .iter() .map(|skill| skill.path_to_skills_md.clone()) .collect(); - file_systems_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); - outcome.file_systems_by_skill_path = SkillFileSystemsByPath::new(file_systems_by_skill_path); + sources_by_skill_path.retain(|path, _| retained_skill_paths.contains(path)); + outcome.sources_by_skill_path = SkillSourcesByPath::new(sources_by_skill_path); fn scope_rank(scope: SkillScope) -> u8 { // Higher-priority scopes first (matches root scan order for dedupe). @@ -212,10 +210,12 @@ pub(crate) async fn skill_roots( ) -> Vec { let home_dir = home_dir().and_then(|path| AbsolutePathBuf::from_absolute_path_checked(path).ok()); + let fs = fs.unwrap_or_else(|| Arc::clone(&LOCAL_FS)); + let cwd = ExecutorPath::new(Arc::clone(&fs), cwd.clone()); skill_roots_with_home_dir( - fs, + &fs, + Some(&cwd), config_layer_stack, - cwd, home_dir.as_ref(), plugin_skill_roots, ) @@ -223,27 +223,27 @@ pub(crate) async fn skill_roots( } async fn skill_roots_with_home_dir( - fs: Option>, + fs: &Arc, + cwd: Option<&ExecutorPath>, config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, home_dir: Option<&AbsolutePathBuf>, plugin_skill_roots: Vec, ) -> Vec { - let mut roots = skill_roots_from_layer_stack_inner(config_layer_stack, home_dir, fs.clone()); + let mut roots = skill_roots_from_layer_stack_inner(fs, config_layer_stack, home_dir, cwd); roots.extend(plugin_skill_roots.into_iter().map(|path| SkillRoot { - path, + path: ExecutorPath::new(Arc::clone(fs), path), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), })); - roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd).await); + roots.extend(repo_agents_skill_roots(cwd, config_layer_stack).await); dedupe_skill_roots_by_path(&mut roots); roots } fn skill_roots_from_layer_stack_inner( + fs: &Arc, config_layer_stack: &ConfigLayerStack, home_dir: Option<&AbsolutePathBuf>, - repo_fs: Option>, + repo_cwd: Option<&ExecutorPath>, ) -> Vec { let mut roots = Vec::new(); @@ -257,11 +257,10 @@ fn skill_roots_from_layer_stack_inner( match &layer.name { ConfigLayerSource::Project { .. } => { - if let Some(repo_fs) = &repo_fs { + if let Some(repo_cwd) = repo_cwd { roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), + path: repo_cwd.with_path(config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::Repo, - file_system: Arc::clone(repo_fs), }); } } @@ -269,35 +268,34 @@ fn skill_roots_from_layer_stack_inner( // Deprecated user skills location (`$CODEX_HOME/skills`), kept for backward // compatibility. roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), + path: ExecutorPath::new(Arc::clone(fs), config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), }); // `$HOME/.agents/skills` (user-installed skills). if let Some(home_dir) = home_dir { roots.push(SkillRoot { - path: home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + path: ExecutorPath::new( + Arc::clone(fs), + home_dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME), + ), scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), }); } // Embedded system skills are cached under `$CODEX_HOME/skills/.system` and are a // special case (not a config layer). roots.push(SkillRoot { - path: system_cache_root_dir(&config_folder), + path: ExecutorPath::new(Arc::clone(fs), system_cache_root_dir(&config_folder)), scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), }); } ConfigLayerSource::System { .. } => { // The system config layer lives under `/etc/codex/` on Unix, so treat // `/etc/codex/skills` as admin-scoped skills. roots.push(SkillRoot { - path: config_folder.join(SKILLS_DIR_NAME), + path: ExecutorPath::new(Arc::clone(fs), config_folder.join(SKILLS_DIR_NAME)), scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), }); } ConfigLayerSource::Mdm { .. } @@ -311,27 +309,24 @@ fn skill_roots_from_layer_stack_inner( } async fn repo_agents_skill_roots( - fs: Option>, + cwd: Option<&ExecutorPath>, config_layer_stack: &ConfigLayerStack, - cwd: &AbsolutePathBuf, ) -> Vec { - let Some(fs) = fs else { + let Some(cwd) = cwd else { return Vec::new(); }; let project_root_markers = project_root_markers_from_stack(config_layer_stack); - let project_root = find_project_root(fs.as_ref(), cwd, &project_root_markers).await; + let project_root = find_project_root(cwd, &project_root_markers).await; let dirs = dirs_between_project_root_and_cwd(cwd, &project_root); let mut roots = Vec::new(); for dir in dirs { let agents_skills = dir.join(AGENTS_DIR_NAME).join(SKILLS_DIR_NAME); - match fs.get_metadata(&agents_skills, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_directory => roots.push(SkillRoot { + match agents_skills.unsandboxed().is_dir().await { + Ok(true) => roots.push(SkillRoot { path: agents_skills, scope: SkillScope::Repo, - file_system: Arc::clone(&fs), }), - Ok(_) => {} - Err(err) if err.kind() == io::ErrorKind::NotFound => {} + Ok(false) => {} Err(err) => { tracing::warn!( "failed to stat repo skills root {}: {err:#}", @@ -365,21 +360,17 @@ fn project_root_markers_from_stack(config_layer_stack: &ConfigLayerStack) -> Vec } } -async fn find_project_root( - fs: &dyn ExecutorFileSystem, - cwd: &AbsolutePathBuf, - project_root_markers: &[String], -) -> AbsolutePathBuf { +async fn find_project_root(cwd: &ExecutorPath, project_root_markers: &[String]) -> AbsolutePathBuf { if project_root_markers.is_empty() { - return cwd.clone(); + return cwd.path().clone(); } for ancestor in cwd.ancestors() { for marker in project_root_markers { let marker_path = ancestor.join(marker); - match fs.get_metadata(&marker_path, /*sandbox*/ None).await { - Ok(_) => return ancestor, - Err(err) if err.kind() == io::ErrorKind::NotFound => {} + match marker_path.unsandboxed().exists().await { + Ok(true) => return ancestor.into_path(), + Ok(false) => {} Err(err) => { tracing::warn!( "failed to stat project root marker {}: {err:#}", @@ -390,20 +381,20 @@ async fn find_project_root( } } - cwd.clone() + cwd.path().clone() } fn dirs_between_project_root_and_cwd( - cwd: &AbsolutePathBuf, + cwd: &ExecutorPath, project_root: &AbsolutePathBuf, -) -> Vec { +) -> Vec { let mut dirs = cwd .ancestors() .scan(false, |done, dir| { if *done { None } else { - if &dir == project_root { + if dir.path() == project_root { *done = true; } Some(dir) @@ -416,25 +407,31 @@ fn dirs_between_project_root_and_cwd( fn dedupe_skill_roots_by_path(roots: &mut Vec) { let mut seen: HashSet = HashSet::new(); - roots.retain(|root| seen.insert(root.path.clone())); + roots.retain(|root| seen.insert(root.path.path().clone())); } -fn canonicalize_for_skill_identity(path: &AbsolutePathBuf) -> AbsolutePathBuf { - path.canonicalize().unwrap_or_else(|_| path.clone()) +fn canonicalize_for_skill_identity(path: &ExecutorPath) -> ExecutorPath { + if !path.is_same_file_system(&LOCAL_FS) { + return path.clone(); + } + + path.with_path( + path.path() + .canonicalize() + .unwrap_or_else(|_| path.path().clone()), + ) } async fn discover_skills_under_root( - fs: &dyn ExecutorFileSystem, - root: &AbsolutePathBuf, + root: &ExecutorPath, scope: SkillScope, outcome: &mut SkillLoadOutcome, ) { let root = canonicalize_for_skill_identity(root); - match fs.get_metadata(&root, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_directory => {} - Ok(_) => return, - Err(err) if err.kind() == io::ErrorKind::NotFound => return, + match root.unsandboxed().is_dir().await { + Ok(true) => {} + Ok(false) => return, Err(err) => { error!("failed to stat skills root {}: {err:#}", root.display()); return; @@ -442,10 +439,10 @@ async fn discover_skills_under_root( } fn enqueue_dir( - queue: &mut VecDeque<(AbsolutePathBuf, usize)>, + queue: &mut VecDeque<(ExecutorPath, usize)>, visited_dirs: &mut HashSet, truncated_by_dir_limit: &mut bool, - path: AbsolutePathBuf, + path: ExecutorPath, depth: usize, ) { if depth > MAX_SCAN_DEPTH { @@ -455,7 +452,7 @@ async fn discover_skills_under_root( *truncated_by_dir_limit = true; return; } - if visited_dirs.insert(path.clone()) { + if visited_dirs.insert(path.path().clone()) { queue.push_back((path, depth)); } } @@ -467,13 +464,13 @@ async fn discover_skills_under_root( ); let mut visited_dirs: HashSet = HashSet::new(); - visited_dirs.insert(root.clone()); + visited_dirs.insert(root.path().clone()); - let mut queue: VecDeque<(AbsolutePathBuf, usize)> = VecDeque::from([(root.clone(), 0)]); + let mut queue: VecDeque<(ExecutorPath, usize)> = VecDeque::from([(root.clone(), 0)]); let mut truncated_by_dir_limit = false; while let Some((dir, depth)) = queue.pop_front() { - let entries = match fs.read_directory(&dir, /*sandbox*/ None).await { + let entries = match dir.unsandboxed().read_directory().await { Ok(entries) => entries, Err(e) => { error!("failed to read skills dir {}: {e:#}", dir.display()); @@ -488,7 +485,7 @@ async fn discover_skills_under_root( } let path = dir.join(&file_name); - let metadata = match fs.get_metadata(&path, /*sandbox*/ None).await { + let metadata = match path.unsandboxed().get_metadata().await { Ok(metadata) => metadata, Err(e) => { error!("failed to stat skills path {}: {e:#}", path.display()); @@ -500,8 +497,8 @@ async fn discover_skills_under_root( if !follow_symlinks { continue; } - match fs.read_directory(&path, /*sandbox*/ None).await { - Ok(_) => { + match path.unsandboxed().is_dir().await { + Ok(true) => { let resolved_dir = canonicalize_for_skill_identity(&path); enqueue_dir( &mut queue, @@ -511,11 +508,7 @@ async fn discover_skills_under_root( depth + 1, ); } - Err(err) - if matches!( - err.kind(), - io::ErrorKind::NotADirectory | io::ErrorKind::NotFound - ) => {} + Ok(false) => {} Err(err) => { error!( "failed to read skills symlink dir {}: {err:#}", @@ -539,14 +532,14 @@ async fn discover_skills_under_root( } if metadata.is_file && file_name == SKILLS_FILENAME { - match parse_skill_file(fs, &path, scope).await { + match parse_skill_file(&path, scope).await { Ok(skill) => { outcome.skills.push(skill); } Err(err) => { if scope != SkillScope::System { outcome.errors.push(SkillError { - path: path.clone(), + path: path.path().clone(), message: err.to_string(), }); } @@ -566,12 +559,12 @@ async fn discover_skills_under_root( } async fn parse_skill_file( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, + path: &ExecutorPath, scope: SkillScope, ) -> Result { - let contents = fs - .read_file_text(path, /*sandbox*/ None) + let contents = path + .unsandboxed() + .read_file_text() .await .map_err(SkillParseError::Read)?; @@ -585,8 +578,8 @@ async fn parse_skill_file( .as_deref() .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; + .unwrap_or_else(|| default_skill_name(path.path())); + let name = namespaced_skill_name(path, &base_name).await; let description = parsed .description .as_deref() @@ -602,7 +595,7 @@ async fn parse_skill_file( interface, dependencies, policy, - } = load_skill_metadata(fs, path).await; + } = load_skill_metadata(path).await; validate_len(&name, MAX_NAME_LEN, "name")?; validate_len(&description, MAX_DESCRIPTION_LEN, "description")?; @@ -623,7 +616,7 @@ async fn parse_skill_file( interface, dependencies, policy, - path_to_skills_md: resolved_path, + path_to_skills_md: resolved_path.into_path(), scope, }) } @@ -640,21 +633,14 @@ fn default_skill_name(path: &AbsolutePathBuf) -> String { .unwrap_or_else(|| "skill".to_string()) } -async fn namespaced_skill_name( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, - base_name: &str, -) -> String { - plugin_namespace_for_skill_path(fs, path) +async fn namespaced_skill_name(path: &ExecutorPath, base_name: &str) -> String { + plugin_namespace_for_skill_path(&path.as_ref()) .await .map(|namespace| format!("{namespace}:{base_name}")) .unwrap_or_else(|| base_name.to_string()) } -async fn load_skill_metadata( - fs: &dyn ExecutorFileSystem, - skill_path: &AbsolutePathBuf, -) -> LoadedSkillMetadata { +async fn load_skill_metadata(skill_path: &ExecutorPath) -> LoadedSkillMetadata { // Fail open: optional metadata should not block loading SKILL.md. let Some(skill_dir) = skill_path.parent() else { return LoadedSkillMetadata::default(); @@ -662,12 +648,9 @@ async fn load_skill_metadata( let metadata_path = skill_dir .join(SKILLS_METADATA_DIR) .join(SKILLS_METADATA_FILENAME); - match fs.get_metadata(&metadata_path, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_file => {} - Ok(_) => return LoadedSkillMetadata::default(), - Err(error) if error.kind() == io::ErrorKind::NotFound => { - return LoadedSkillMetadata::default(); - } + match metadata_path.unsandboxed().is_file().await { + Ok(true) => {} + Ok(false) => return LoadedSkillMetadata::default(), Err(error) => { tracing::warn!( "ignoring {path}: failed to stat {label}: {error}", @@ -678,7 +661,7 @@ async fn load_skill_metadata( } } - let contents = match fs.read_file_text(&metadata_path, /*sandbox*/ None).await { + let contents = match metadata_path.unsandboxed().read_file_text().await { Ok(contents) => contents, Err(error) => { tracing::warn!( @@ -691,7 +674,7 @@ async fn load_skill_metadata( }; let parsed: SkillMetadataFile = { - let _guard = AbsolutePathBufGuard::new(skill_dir.as_path()); + let _guard = AbsolutePathBufGuard::new(skill_dir.path().as_path()); match serde_yaml::from_str(&contents) { Ok(parsed) => parsed, Err(error) => { @@ -711,7 +694,7 @@ async fn load_skill_metadata( policy, } = parsed; LoadedSkillMetadata { - interface: resolve_interface(interface, &skill_dir), + interface: resolve_interface(interface, skill_dir.path()), dependencies: resolve_dependencies(dependencies), policy: resolve_policy(policy), } @@ -950,7 +933,8 @@ pub(crate) async fn skill_roots_from_layer_stack( cwd: &AbsolutePathBuf, home_dir: Option<&AbsolutePathBuf>, ) -> Vec { - skill_roots_with_home_dir(Some(fs), config_layer_stack, cwd, home_dir, Vec::new()).await + let cwd = ExecutorPath::new(fs, cwd.clone()); + skill_roots_with_home_dir(Some(&cwd), config_layer_stack, home_dir, Vec::new()).await } #[cfg(test)] diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index a12f09f80f..efe9ffc487 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,6 +4,7 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_exec_server::ExecutorPath; use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; @@ -26,6 +27,13 @@ struct TestConfig { config_layer_stack: ConfigLayerStack, } +fn local_skill_root(path: AbsolutePathBuf, scope: SkillScope) -> SkillRoot { + SkillRoot { + path: ExecutorPath::new(Arc::clone(&LOCAL_FS), path), + scope, + } +} + async fn make_config(codex_home: &TempDir) -> TestConfig { make_config_for_cwd(codex_home, codex_home.path().to_path_buf()).await } @@ -932,12 +940,9 @@ async fn loads_skills_via_symlinked_subdir_for_admin_scope() { fs::create_dir_all(admin_root.path()).unwrap(); symlink_dir(shared.path(), &admin_root.path().join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: admin_root.path().abs(), - scope: SkillScope::Admin, - file_system: Arc::clone(&LOCAL_FS), - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(admin_root.path().abs(), SkillScope::Admin)]) + .await; assert!( outcome.errors.is_empty(), @@ -1010,12 +1015,8 @@ async fn system_scope_ignores_symlinked_subdir() { fs::create_dir_all(&system_root).unwrap(); symlink_dir(shared.path(), &system_root.join("shared")); - let outcome = load_skills_from_roots([SkillRoot { - path: system_root.abs(), - scope: SkillScope::System, - file_system: Arc::clone(&LOCAL_FS), - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(system_root.abs(), SkillScope::System)]).await; assert!( outcome.errors.is_empty(), "unexpected errors: {:?}", @@ -1042,12 +1043,8 @@ async fn respects_max_scan_depth_for_user_scope() { ); let skills_root = codex_home.path().join("skills"); - let outcome = load_skills_from_roots([SkillRoot { - path: skills_root.abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - }]) - .await; + let outcome = + load_skills_from_roots([local_skill_root(skills_root.abs(), SkillScope::User)]).await; assert!( outcome.errors.is_empty(), @@ -1144,11 +1141,10 @@ async fn namespaces_plugin_skills_using_plugin_name() { ) .unwrap(); - let outcome = load_skills_from_roots([SkillRoot { - path: plugin_root.join("skills").abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - }]) + let outcome = load_skills_from_roots([local_skill_root( + plugin_root.join("skills").abs(), + SkillScope::User, + )]) .await; assert!( @@ -1458,16 +1454,8 @@ async fn deduplicates_by_path_preferring_first_root() { let skill_path = write_skill_at(root.path(), "dupe", "dupe-skill", "from repo"); let outcome = load_skills_from_roots([ - SkillRoot { - path: root.path().abs(), - scope: SkillScope::Repo, - file_system: Arc::clone(&LOCAL_FS), - }, - SkillRoot { - path: root.path().abs(), - scope: SkillScope::User, - file_system: Arc::clone(&LOCAL_FS), - }, + local_skill_root(root.path().abs(), SkillScope::Repo), + local_skill_root(root.path().abs(), SkillScope::User), ]) .await; diff --git a/codex-rs/core-skills/src/manager.rs b/codex-rs/core-skills/src/manager.rs index b7b7a4b64d..a7b3889947 100644 --- a/codex-rs/core-skills/src/manager.rs +++ b/codex-rs/core-skills/src/manager.rs @@ -5,6 +5,8 @@ use std::sync::RwLock; use codex_config::ConfigLayerStack; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPath; +use codex_exec_server::LOCAL_FS; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -83,9 +85,9 @@ impl SkillsManager { /// Load skills for an already-constructed [`Config`], avoiding any additional config-layer /// loading. /// - /// This path uses a cache keyed by the effective skill-relevant config state rather than just - /// cwd so role-local and session-local skill overrides cannot bleed across sessions that happen - /// to share a directory. + /// This path uses a cache keyed by the effective skill-relevant config state for local + /// filesystem roots. Executor-backed roots skip that cache because an absolute path alone is + /// not a stable executor identity. pub async fn skills_for_config( &self, input: &SkillsLoadInput, @@ -93,17 +95,27 @@ impl SkillsManager { ) -> SkillLoadOutcome { let roots = self.skill_roots_for_config(input, fs).await; let skill_config_rules = skill_config_rules_from_stack(&input.config_layer_stack); - let cache_key = config_skills_cache_key(&roots, &skill_config_rules); - if let Some(outcome) = self.cached_outcome_for_config(&cache_key) { - return outcome; - } + let use_config_cache = roots + .iter() + .all(|root| root.path.is_same_file_system(&LOCAL_FS)); + let cache_key = if use_config_cache { + let cache_key = config_skills_cache_key(&roots, &skill_config_rules); + if let Some(outcome) = self.cached_outcome_for_config(&cache_key) { + return outcome; + } + Some(cache_key) + } else { + None + }; let outcome = self.build_skill_outcome(roots, &skill_config_rules).await; - let mut cache = self - .cache_by_config - .write() - .unwrap_or_else(std::sync::PoisonError::into_inner); - cache.insert(cache_key, outcome.clone()); + if let Some(cache_key) = cache_key { + let mut cache = self + .cache_by_config + .write() + .unwrap_or_else(std::sync::PoisonError::into_inner); + cache.insert(cache_key, outcome.clone()); + } outcome } @@ -142,7 +154,9 @@ impl SkillsManager { extra_user_roots: &[AbsolutePathBuf], fs: Option>, ) -> SkillLoadOutcome { - let use_cwd_cache = fs.is_some(); + // The cwd cache key is only an absolute path, so it is safe for the + // process-local filesystem but ambiguous across executor filesystems. + let use_cwd_cache = fs.as_ref().is_some_and(|fs| Arc::ptr_eq(fs, &*LOCAL_FS)); if use_cwd_cache && !force_reload && let Some(outcome) = self.cached_outcome_for_cwd(&input.cwd) @@ -165,9 +179,8 @@ impl SkillsManager { normalize_extra_user_roots(extra_user_roots) .into_iter() .map(|path| SkillRoot { - path, + path: ExecutorPath::new(Arc::clone(&fs), path), scope: SkillScope::User, - file_system: Arc::clone(&fs), }), ); } @@ -279,7 +292,7 @@ fn config_skills_cache_key( SkillScope::System => 2, SkillScope::Admin => 3, }; - (root.path.clone(), scope_rank) + (root.path.path().clone(), scope_rank) }) .collect(), skill_config_rules: skill_config_rules.clone(), diff --git a/codex-rs/core-skills/src/model.rs b/codex-rs/core-skills/src/model.rs index eb9a6f132f..58cd1b8655 100644 --- a/codex-rs/core-skills/src/model.rs +++ b/codex-rs/core-skills/src/model.rs @@ -3,7 +3,7 @@ use std::collections::HashSet; use std::fmt; use std::sync::Arc; -use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPath; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -89,7 +89,7 @@ pub struct SkillLoadOutcome { pub skills: Vec, pub errors: Vec, pub disabled_paths: HashSet, - pub(crate) file_systems_by_skill_path: SkillFileSystemsByPath, + pub(crate) sources_by_skill_path: SkillSourcesByPath, pub(crate) implicit_skills_by_scripts_dir: Arc>, pub(crate) implicit_skills_by_doc_path: Arc>, } @@ -117,29 +117,25 @@ impl SkillLoadOutcome { .map(|skill| (skill, self.is_skill_enabled(skill))) } - pub(crate) fn file_system_for_skill( - &self, - skill: &SkillMetadata, - ) -> Option> { - self.file_systems_by_skill_path - .get(&skill.path_to_skills_md) + pub(crate) fn source_for_skill(&self, skill: &SkillMetadata) -> Option { + self.sources_by_skill_path.get(&skill.path_to_skills_md) } } #[derive(Clone, Default)] -pub(crate) struct SkillFileSystemsByPath { - values: Arc>>, +pub(crate) struct SkillSourcesByPath { + values: Arc>, } -impl SkillFileSystemsByPath { - pub(crate) fn new(values: HashMap>) -> Self { +impl SkillSourcesByPath { + pub(crate) fn new(values: HashMap) -> Self { Self { values: Arc::new(values), } } - fn get(&self, path: &AbsolutePathBuf) -> Option> { - self.values.get(path).map(Arc::clone) + fn get(&self, path: &AbsolutePathBuf) -> Option { + self.values.get(path).cloned() } fn retain_paths(&mut self, paths: &HashSet) { @@ -147,15 +143,15 @@ impl SkillFileSystemsByPath { self.values .iter() .filter(|(path, _)| paths.contains(*path)) - .map(|(path, fs)| (path.clone(), Arc::clone(fs))) + .map(|(path, source)| (path.clone(), source.clone())) .collect(), ); } } -impl fmt::Debug for SkillFileSystemsByPath { +impl fmt::Debug for SkillSourcesByPath { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - f.debug_struct("SkillFileSystemsByPath") + f.debug_struct("SkillSourcesByPath") .field("len", &self.values.len()) .finish() } @@ -173,9 +169,7 @@ pub fn filter_skill_load_outcome_for_product( .iter() .map(|skill| skill.path_to_skills_md.clone()) .collect(); - outcome - .file_systems_by_skill_path - .retain_paths(&retained_paths); + outcome.sources_by_skill_path.retain_paths(&retained_paths); outcome.implicit_skills_by_scripts_dir = Arc::new( outcome .implicit_skills_by_scripts_dir diff --git a/codex-rs/core/src/agents_md.rs b/codex-rs/core/src/agents_md.rs index a1a883e839..b16da16e98 100644 --- a/codex-rs/core/src/agents_md.rs +++ b/codex-rs/core/src/agents_md.rs @@ -23,9 +23,9 @@ use crate::config_loader::project_root_markers_from_config; use codex_app_server_protocol::ConfigLayerSource; use codex_exec_server::Environment; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPathRef; use codex_features::Feature; use codex_utils_absolute_path::AbsolutePathBuf; -use dunce::canonicalize as normalize_path; use std::io; use toml::Value as TomlValue; use tracing::error; @@ -126,7 +126,8 @@ impl<'a> AgentsMdManager<'a> { &self, fs: &dyn ExecutorFileSystem, ) -> Option { - let agents_md_docs = self.read_agents_md(fs).await; + let cwd = self.executor_cwd(fs); + let agents_md_docs = self.read_agents_md(&cwd).await; let mut output = String::new(); @@ -173,8 +174,11 @@ impl<'a> AgentsMdManager<'a> { let mut paths = Self::load_global_instructions(Some(&self.config.codex_home)) .map(|loaded| vec![loaded.path]) .unwrap_or_default(); - match self.agents_md_paths(fs).await { - Ok(agents_md_paths) => paths.extend(agents_md_paths), + let cwd = self.executor_cwd(fs); + match self.agents_md_paths(&cwd).await { + Ok(agents_md_paths) => { + paths.extend(agents_md_paths.into_iter().map(|path| path.path().clone())); + } Err(err) => { tracing::warn!(error = %err, "failed to discover AGENTS.md docs for instruction sources"); } @@ -188,14 +192,14 @@ impl<'a> AgentsMdManager<'a> { /// concatenation of all discovered docs. If no documentation file is found /// the function returns `Ok(None)`. Unexpected I/O failures bubble up as /// `Err` so callers can decide how to handle them. - async fn read_agents_md(&self, fs: &dyn ExecutorFileSystem) -> io::Result> { + async fn read_agents_md(&self, cwd: &ExecutorPathRef<'_>) -> io::Result> { let max_total = self.config.project_doc_max_bytes; if max_total == 0 { return Ok(None); } - let paths = self.agents_md_paths(fs).await?; + let paths = self.agents_md_paths(cwd).await?; if paths.is_empty() { return Ok(None); } @@ -208,14 +212,14 @@ impl<'a> AgentsMdManager<'a> { break; } - match fs.get_metadata(&p, /*sandbox*/ None).await { + match p.unsandboxed().get_metadata().await { Ok(metadata) if !metadata.is_file => continue, Ok(_) => {} Err(err) if err.kind() == io::ErrorKind::NotFound => continue, Err(err) => return Err(err), } - let mut data = match fs.read_file(&p, /*sandbox*/ None).await { + let mut data = match p.unsandboxed().read_file().await { Ok(data) => data, Err(err) if err.kind() == io::ErrorKind::NotFound => continue, Err(err) => return Err(err), @@ -252,19 +256,14 @@ impl<'a> AgentsMdManager<'a> { /// contents. The list is ordered from project root to the current working /// directory (inclusive). Symlinks are allowed. When `project_doc_max_bytes` /// is zero, returns an empty list. - async fn agents_md_paths( + async fn agents_md_paths<'fs>( &self, - fs: &dyn ExecutorFileSystem, - ) -> io::Result> { + cwd: &ExecutorPathRef<'fs>, + ) -> io::Result>> { if self.config.project_doc_max_bytes == 0 { return Ok(Vec::new()); } - let mut dir = self.config.cwd.clone(); - if let Ok(canon) = normalize_path(&dir) { - dir = AbsolutePathBuf::try_from(canon)?; - } - let mut merged = TomlValue::Table(toml::map::Map::new()); for layer in self.config.config_layer_stack.get_layers( ConfigLayerStackOrdering::LowestPrecedenceFirst, @@ -285,11 +284,10 @@ impl<'a> AgentsMdManager<'a> { }; let mut project_root = None; if !project_root_markers.is_empty() { - for ancestor in dir.ancestors() { + for ancestor in cwd.ancestors() { for marker in &project_root_markers { let marker_path = ancestor.join(marker); - let marker_exists = match fs.get_metadata(&marker_path, /*sandbox*/ None).await - { + let marker_exists = match marker_path.unsandboxed().get_metadata().await { Ok(_) => true, Err(err) if err.kind() == io::ErrorKind::NotFound => false, Err(err) => return Err(err), @@ -305,12 +303,12 @@ impl<'a> AgentsMdManager<'a> { } } - let search_dirs: Vec = if let Some(root) = project_root { + let search_dirs: Vec> = if let Some(root) = project_root { let mut dirs = Vec::new(); - let mut cursor = dir.clone(); + let mut cursor = cwd.clone(); loop { dirs.push(cursor.clone()); - if cursor == root { + if cursor.path() == root.path() { break; } let Some(parent) = cursor.parent() else { @@ -321,15 +319,15 @@ impl<'a> AgentsMdManager<'a> { dirs.reverse(); dirs } else { - vec![dir] + vec![cwd.clone()] }; - let mut found: Vec = Vec::new(); + let mut found: Vec> = Vec::new(); let candidate_filenames = self.candidate_filenames(); for d in search_dirs { for name in &candidate_filenames { let candidate = d.join(name); - match fs.get_metadata(&candidate, /*sandbox*/ None).await { + match candidate.unsandboxed().get_metadata().await { Ok(md) if md.is_file => { found.push(candidate); break; @@ -344,6 +342,10 @@ impl<'a> AgentsMdManager<'a> { Ok(found) } + fn executor_cwd<'fs>(&self, fs: &'fs dyn ExecutorFileSystem) -> ExecutorPathRef<'fs> { + ExecutorPathRef::new(fs, self.config.cwd.clone()) + } + fn candidate_filenames(&self) -> Vec<&str> { let mut names: Vec<&str> = Vec::with_capacity(2 + self.config.project_doc_fallback_filenames.len()); diff --git a/codex-rs/core/src/codex/handlers.rs b/codex-rs/core/src/codex/handlers.rs index 7e0ba45dad..55d3af5f1f 100644 --- a/codex-rs/core/src/codex/handlers.rs +++ b/codex-rs/core/src/codex/handlers.rs @@ -16,8 +16,9 @@ use crate::codex::SteerInputError; use crate::codex::spawn_review_thread; use crate::config::Config; use crate::config_loader::CloudRequirementsLoader; +use crate::config_loader::ConfigLoadFileSystems; use crate::config_loader::LoaderOverrides; -use crate::config_loader::load_config_layers_state; +use crate::config_loader::load_config_layers_state_with_file_systems; use crate::realtime_context::REALTIME_TURN_TOKEN_BUDGET; use crate::realtime_context::truncate_realtime_text_to_token_budget; use crate::realtime_conversation::REALTIME_USER_TEXT_PREFIX; @@ -501,17 +502,23 @@ pub async fn list_skills(sess: &Session, sub_id: String, cwds: Vec, for let skills_manager = &sess.services.skills_manager; let plugins_manager = &sess.services.plugins_manager; - let fs = sess + let (fs, remote_project_fs) = sess .services .environment .as_ref() - .map(|environment| environment.get_filesystem()); + .map(|environment| (environment.get_filesystem(), environment.is_remote())) + .unwrap_or_else(|| (Arc::clone(&LOCAL_FS), false)); let config = sess.get_config().await; let codex_home = sess.codex_home().await; let mut skills = Vec::new(); let empty_cli_overrides: &[(String, toml::Value)] = &[]; for cwd in cwds { - let cwd_abs = match AbsolutePathBuf::relative_to_current_dir(cwd.as_path()) { + let cwd_abs = if remote_project_fs { + AbsolutePathBuf::from_absolute_path_checked(cwd.as_path()) + } else { + AbsolutePathBuf::relative_to_current_dir(cwd.as_path()) + }; + let cwd_abs = match cwd_abs { Ok(path) => path, Err(err) => { let error_path = cwd.clone(); @@ -526,8 +533,8 @@ pub async fn list_skills(sess: &Session, sub_id: String, cwds: Vec, for continue; } }; - let config_layer_stack = match load_config_layers_state( - LOCAL_FS.as_ref(), + let config_layer_stack = match load_config_layers_state_with_file_systems( + ConfigLoadFileSystems::same(fs.as_ref()), &codex_home, Some(cwd_abs.clone()), empty_cli_overrides, @@ -563,7 +570,7 @@ pub async fn list_skills(sess: &Session, sub_id: String, cwds: Vec, for config.bundled_skills_enabled(), ); let outcome = skills_manager - .skills_for_cwd(&skills_input, force_reload, fs.clone()) + .skills_for_cwd(&skills_input, force_reload, Some(Arc::clone(&fs))) .await; let errors = super::errors_to_info(&outcome.errors); let skills_metadata = super::skills_to_info(&outcome.skills, &outcome.disabled_paths); diff --git a/codex-rs/core/src/codex/turn.rs b/codex-rs/core/src/codex/turn.rs index fa24bd6924..a9e95bb9f2 100644 --- a/codex-rs/core/src/codex/turn.rs +++ b/codex-rs/core/src/codex/turn.rs @@ -243,6 +243,11 @@ pub(crate) async fn run_turn( } = build_skill_injections( &mentioned_skills, skills_outcome, + turn_context + .environment + .as_ref() + .map(|environment| environment.get_filesystem()) + .unwrap_or_else(|| Arc::clone(&codex_exec_server::LOCAL_FS)), Some(&session_telemetry), &sess.services.analytics_events_client, tracking.clone(), diff --git a/codex-rs/core/src/config/agent_roles.rs b/codex-rs/core/src/config/agent_roles.rs index b1d28cf838..e6171181bd 100644 --- a/codex-rs/core/src/config/agent_roles.rs +++ b/codex-rs/core/src/config/agent_roles.rs @@ -5,6 +5,7 @@ use codex_config::config_toml::AgentRoleToml; use codex_config::config_toml::AgentsToml; use codex_config::config_toml::ConfigToml; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPathRef; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::AbsolutePathBufGuard; use serde::Deserialize; @@ -70,13 +71,10 @@ pub(crate) async fn load_agent_roles( } if let Some(config_folder) = layer.config_folder() { - for (role_name, role) in discover_agent_roles_in_dir( - fs, - &config_folder.join("agents"), - &declared_role_files, - startup_warnings, - ) - .await? + let agents_dir = ExecutorPathRef::new(fs, config_folder.join("agents")); + for (role_name, role) in + discover_agent_roles_in_dir(&agents_dir, &declared_role_files, startup_warnings) + .await? { if layer_roles.contains_key(&role_name) { push_agent_role_warning( @@ -150,8 +148,9 @@ async fn read_declared_role( let mut role_name = declared_role_name.to_string(); if let Some(config_file) = role.config_file.as_deref() { let config_file = AbsolutePathBuf::from_absolute_path(config_file)?; + let config_file = ExecutorPathRef::new(fs, config_file); let parsed_file = - read_resolved_agent_role_file(fs, &config_file, Some(declared_role_name)).await?; + read_resolved_agent_role_file(&config_file, Some(declared_role_name)).await?; role_name = parsed_file.role_name; role.description = parsed_file.description.or(role.description); role.nickname_candidates = parsed_file.nickname_candidates.or(role.nickname_candidates); @@ -191,7 +190,10 @@ async fn agent_role_config_from_toml( .as_ref() .map(AbsolutePathBuf::from_absolute_path) .transpose()?; - validate_agent_role_config_file(fs, role_name, config_file.as_ref()).await?; + let config_file_for_validation = config_file + .as_ref() + .map(|config_file| ExecutorPathRef::new(fs, config_file.clone())); + validate_agent_role_config_file(role_name, config_file_for_validation.as_ref()).await?; let description = normalize_agent_role_description( &format!("agents.{role_name}.description"), role.description.as_deref(), @@ -309,16 +311,15 @@ pub(crate) fn parse_agent_role_file_contents( } async fn read_resolved_agent_role_file( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, + path: &ExecutorPathRef<'_>, role_name_hint: Option<&str>, ) -> std::io::Result { - let contents = fs.read_file_text(path, /*sandbox*/ None).await?; + let contents = path.unsandboxed().read_file_text().await?; let config_base_dir = path.parent().unwrap_or_else(|| path.clone()); parse_agent_role_file_contents( &contents, - path.as_path(), - config_base_dir.as_path(), + path.path().as_path(), + config_base_dir.path().as_path(), role_name_hint, ) } @@ -377,23 +378,23 @@ fn validate_agent_role_file_developer_instructions( } async fn validate_agent_role_config_file( - fs: &dyn ExecutorFileSystem, role_name: &str, - config_file: Option<&AbsolutePathBuf>, + config_file: Option<&ExecutorPathRef<'_>>, ) -> std::io::Result<()> { let Some(config_file) = config_file else { return Ok(()); }; - let metadata = fs - .get_metadata(config_file, /*sandbox*/ None) + let metadata = config_file + .unsandboxed() + .get_metadata() .await .map_err(|e| { std::io::Error::new( std::io::ErrorKind::InvalidInput, format!( "agents.{role_name}.config_file must point to an existing file at {}: {e}", - config_file.as_path().display() + config_file.display() ), ) })?; @@ -404,7 +405,7 @@ async fn validate_agent_role_config_file( std::io::ErrorKind::InvalidInput, format!( "agents.{role_name}.config_file must point to a file: {}", - config_file.as_path().display() + config_file.display() ), )) } @@ -463,19 +464,18 @@ fn normalize_agent_role_nickname_candidates( } async fn discover_agent_roles_in_dir( - fs: &dyn ExecutorFileSystem, - agents_dir: &AbsolutePathBuf, + agents_dir: &ExecutorPathRef<'_>, declared_role_files: &BTreeSet, startup_warnings: &mut Vec, ) -> std::io::Result> { let mut roles = BTreeMap::new(); - for agent_file in collect_agent_role_files(fs, agents_dir).await? { - if declared_role_files.contains(agent_file.as_path()) { + for agent_file in collect_agent_role_files(agents_dir).await? { + if declared_role_files.contains(agent_file.path().as_path()) { continue; } let parsed_file = - match read_resolved_agent_role_file(fs, &agent_file, /*role_name_hint*/ None).await { + match read_resolved_agent_role_file(&agent_file, /*role_name_hint*/ None).await { Ok(parsed_file) => parsed_file, Err(err) => { push_agent_role_warning(startup_warnings, err); @@ -490,7 +490,7 @@ async fn discover_agent_roles_in_dir( std::io::ErrorKind::InvalidInput, format!( "duplicate agent role name `{role_name}` discovered in {}", - agents_dir.as_path().display() + agents_dir.display() ), ), ); @@ -509,14 +509,13 @@ async fn discover_agent_roles_in_dir( Ok(roles) } -async fn collect_agent_role_files( - fs: &dyn ExecutorFileSystem, - dir: &AbsolutePathBuf, -) -> std::io::Result> { +async fn collect_agent_role_files<'fs>( + dir: &ExecutorPathRef<'fs>, +) -> std::io::Result>> { let mut files = Vec::new(); let mut dirs = vec![dir.clone()]; while let Some(dir) = dirs.pop() { - let entries = match fs.read_directory(&dir, /*sandbox*/ None).await { + let entries = match dir.unsandboxed().read_directory().await { Ok(entries) => entries, Err(err) if err.kind() == ErrorKind::NotFound => continue, Err(err) => return Err(err), @@ -530,6 +529,7 @@ async fn collect_agent_role_files( } if entry.is_file && path + .path() .as_path() .extension() .is_some_and(|extension| extension == "toml") @@ -539,6 +539,6 @@ async fn collect_agent_role_files( } } - files.sort(); + files.sort_by(|a, b| a.path().cmp(b.path())); Ok(files) } diff --git a/codex-rs/core/src/config/mod.rs b/codex-rs/core/src/config/mod.rs index 47a8bdf304..f1a63d2b16 100644 --- a/codex-rs/core/src/config/mod.rs +++ b/codex-rs/core/src/config/mod.rs @@ -48,6 +48,7 @@ use codex_config::types::TuiNotificationSettings; use codex_config::types::UriBasedFileOpener; use codex_config::types::WindowsSandboxModeToml; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPathRef; use codex_exec_server::LOCAL_FS; use codex_features::Feature; use codex_features::FeatureConfigSource; @@ -56,7 +57,7 @@ use codex_features::FeatureToml; use codex_features::Features; use codex_features::FeaturesToml; use codex_features::MultiAgentV2ConfigToml; -use codex_git_utils::resolve_root_git_project_for_trust; +use codex_git_utils::resolve_root_git_project_for_trust_at; use codex_login::AuthManagerConfig; use codex_mcp::McpConfig; use codex_model_provider_info::LEGACY_OLLAMA_CHAT_PROVIDER_ID; @@ -1561,7 +1562,8 @@ impl Config { .into_iter() .map(|path| AbsolutePathBuf::resolve_path_against_base(path, resolved_cwd.as_path())) .collect(); - let repo_root = resolve_root_git_project_for_trust(fs, &resolved_cwd).await; + let resolved_cwd_path = ExecutorPathRef::new(fs, resolved_cwd.clone()); + let repo_root = resolve_root_git_project_for_trust_at(&resolved_cwd_path).await; let active_project = cfg .get_active_project( resolved_cwd.as_path(), @@ -2252,16 +2254,14 @@ impl Config { let Some(path) = path else { return Ok(None); }; + let path = ExecutorPathRef::new(fs, path.clone()); - let contents = fs - .read_file_text(path, /*sandbox*/ None) - .await - .map_err(|e| { - std::io::Error::new( - e.kind(), - format!("failed to read {context} {}: {e}", path.display()), - ) - })?; + let contents = path.unsandboxed().read_file_text().await.map_err(|e| { + std::io::Error::new( + e.kind(), + format!("failed to read {context} {}: {e}", path.display()), + ) + })?; let s = contents.trim().to_string(); if s.is_empty() { diff --git a/codex-rs/core/src/config_loader/README.md b/codex-rs/core/src/config_loader/README.md index 44a514a10a..00a13a9321 100644 --- a/codex-rs/core/src/config_loader/README.md +++ b/codex-rs/core/src/config_loader/README.md @@ -11,6 +11,8 @@ This module is the canonical place to **load and describe Codex configuration la Exported from `codex_core::config_loader`: - `load_config_layers_state(fs, codex_home, cwd_opt, cli_overrides, overrides, cloud_requirements) -> ConfigLayerStack` +- `load_config_layers_state_with_file_systems(file_systems, codex_home, cwd_opt, cli_overrides, overrides, cloud_requirements) -> ConfigLayerStack` + - Use this when local config files and project config files live on different executor filesystems. - `ConfigLayerStack` - `effective_config() -> toml::Value` - `origins() -> HashMap` diff --git a/codex-rs/core/src/config_loader/layer_io.rs b/codex-rs/core/src/config_loader/layer_io.rs index 6bd9a9130f..d7a3b154e8 100644 --- a/codex-rs/core/src/config_loader/layer_io.rs +++ b/codex-rs/core/src/config_loader/layer_io.rs @@ -6,6 +6,7 @@ use super::macos::load_managed_admin_config_layer; use codex_config::config_error_from_toml; use codex_config::io_error_from_config_error; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPathRef; use codex_utils_absolute_path::AbsolutePathBuf; use std::io; use std::path::Path; @@ -53,16 +54,19 @@ pub(super) async fn load_config_layers_internal( .. } = overrides; - let managed_config_path = AbsolutePathBuf::from_absolute_path( - managed_config_path.unwrap_or_else(|| managed_config_default_path(codex_home)), - )?; + let managed_config_path = ExecutorPathRef::new( + fs, + AbsolutePathBuf::from_absolute_path( + managed_config_path.unwrap_or_else(|| managed_config_default_path(codex_home)), + )?, + ); let managed_config = - read_config_from_path(fs, &managed_config_path, /*log_missing_as_info*/ false) + read_config_from_path(&managed_config_path, /*log_missing_as_info*/ false) .await? .map(|managed_config| MangedConfigFromFile { managed_config, - file: managed_config_path.clone(), + file: managed_config_path.path().clone(), }); #[cfg(target_os = "macos")] @@ -90,16 +94,16 @@ fn map_managed_admin_layer(layer: ManagedAdminConfigLayer) -> ManagedConfigFromM } pub(super) async fn read_config_from_path( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, + path: &ExecutorPathRef<'_>, log_missing_as_info: bool, ) -> io::Result> { - match fs.read_file_text(path, /*sandbox*/ None).await { + match path.unsandboxed().read_file_text().await { Ok(contents) => match toml::from_str::(&contents) { Ok(value) => Ok(Some(value)), Err(err) => { - tracing::error!("Failed to parse {}: {err}", path.as_path().display()); - let config_error = config_error_from_toml(path.as_path(), &contents, err.clone()); + tracing::error!("Failed to parse {}: {err}", path.display()); + let config_error = + config_error_from_toml(path.path().as_path(), &contents, err.clone()); Err(io_error_from_config_error( io::ErrorKind::InvalidData, config_error, @@ -109,14 +113,14 @@ pub(super) async fn read_config_from_path( }, Err(err) if err.kind() == io::ErrorKind::NotFound => { if log_missing_as_info { - tracing::info!("{} not found, using defaults", path.as_path().display()); + tracing::info!("{} not found, using defaults", path.display()); } else { - tracing::debug!("{} not found", path.as_path().display()); + tracing::debug!("{} not found", path.display()); } Ok(None) } Err(err) => { - tracing::error!("Failed to read {}: {err}", path.as_path().display()); + tracing::error!("Failed to read {}: {err}", path.display()); Err(err) } } diff --git a/codex-rs/core/src/config_loader/mod.rs b/codex-rs/core/src/config_loader/mod.rs index fb02e670ef..5ce0eac453 100644 --- a/codex-rs/core/src/config_loader/mod.rs +++ b/codex-rs/core/src/config_loader/mod.rs @@ -12,7 +12,8 @@ use codex_config::ConfigRequirementsWithSources; use codex_config::config_toml::ConfigToml; use codex_config::config_toml::ProjectConfig; use codex_exec_server::ExecutorFileSystem; -use codex_git_utils::resolve_root_git_project_for_trust; +use codex_exec_server::ExecutorPathRef; +use codex_git_utils::resolve_root_git_project_for_trust_at; use codex_protocol::config_types::ApprovalsReviewer; use codex_protocol::config_types::SandboxMode; use codex_protocol::config_types::TrustLevel; @@ -87,6 +88,26 @@ pub(crate) async fn first_layer_config_error_from_entries( .await } +/// Filesystems used while loading config layers. +/// +/// Local config layers (`/etc`, managed config, and `$CODEX_HOME`) are read +/// from `local`. Project config layers are read from `project` because `cwd` +/// can refer to a different executor filesystem. +#[derive(Clone, Copy)] +pub struct ConfigLoadFileSystems<'a> { + pub local: &'a dyn ExecutorFileSystem, + pub project: &'a dyn ExecutorFileSystem, +} + +impl<'a> ConfigLoadFileSystems<'a> { + pub fn same(file_system: &'a dyn ExecutorFileSystem) -> Self { + Self { + local: file_system, + project: file_system, + } + } +} + /// To build up the set of admin-enforced constraints, we build up from multiple /// configuration layers in the following order, but a constraint defined in an /// earlier layer cannot be overridden by a later layer: @@ -125,6 +146,25 @@ pub async fn load_config_layers_state( cli_overrides: &[(String, TomlValue)], overrides: LoaderOverrides, cloud_requirements: CloudRequirementsLoader, +) -> io::Result { + load_config_layers_state_with_file_systems( + ConfigLoadFileSystems::same(fs), + codex_home, + cwd, + cli_overrides, + overrides, + cloud_requirements, + ) + .await +} + +pub async fn load_config_layers_state_with_file_systems( + file_systems: ConfigLoadFileSystems<'_>, + codex_home: &Path, + cwd: Option, + cli_overrides: &[(String, TomlValue)], + overrides: LoaderOverrides, + cloud_requirements: CloudRequirementsLoader, ) -> io::Result { let mut config_requirements_toml = ConfigRequirementsWithSources::default(); @@ -143,13 +183,14 @@ pub async fn load_config_layers_state( .await?; // Honor the system requirements.toml location. - let requirements_toml_file = system_requirements_toml_file()?; - load_requirements_toml(fs, &mut config_requirements_toml, &requirements_toml_file).await?; + let requirements_toml_file = + ExecutorPathRef::new(file_systems.local, system_requirements_toml_file()?); + load_requirements_toml(&mut config_requirements_toml, &requirements_toml_file).await?; // Make a best-effort to support the legacy `managed_config.toml` as a // requirements specification. let loaded_config_layers = - layer_io::load_config_layers_internal(fs, codex_home, overrides).await?; + layer_io::load_config_layers_internal(file_systems.local, codex_home, overrides).await?; load_requirements_from_legacy_scheme( &mut config_requirements_toml, loaded_config_layers.clone(), @@ -174,12 +215,13 @@ pub async fn load_config_layers_state( // Include an entry for the "system" config folder, loading its config.toml, // if it exists. - let system_config_toml_file = system_config_toml_file()?; + let system_config_toml_file = + ExecutorPathRef::new(file_systems.local, system_config_toml_file()?); let system_layer = - load_config_toml_for_required_layer(fs, &system_config_toml_file, |config_toml| { + load_config_toml_for_required_layer(&system_config_toml_file, |config_toml| { ConfigLayerEntry::new( ConfigLayerSource::System { - file: system_config_toml_file.clone(), + file: system_config_toml_file.path().clone(), }, config_toml, ) @@ -190,11 +232,14 @@ pub async fn load_config_layers_state( // Add a layer for $CODEX_HOME/config.toml if it exists. Note if the file // exists, but is malformed, then this error should be propagated to the // user. - let user_file = AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home); - let user_layer = load_config_toml_for_required_layer(fs, &user_file, |config_toml| { + let user_file = ExecutorPathRef::new( + file_systems.local, + AbsolutePathBuf::resolve_path_against_base(CONFIG_TOML_FILE, codex_home), + ); + let user_layer = load_config_toml_for_required_layer(&user_file, |config_toml| { ConfigLayerEntry::new( ConfigLayerSource::User { - file: user_file.clone(), + file: user_file.path().clone(), }, config_toml, ) @@ -224,13 +269,13 @@ pub async fn load_config_layers_state( return Err(err); } }; + let cwd = ExecutorPathRef::new(file_systems.project, cwd); let project_trust_context = match project_trust_context( - fs, &merged_so_far, &cwd, &project_root_markers, codex_home, - &user_file, + user_file.path(), ) .await { @@ -251,7 +296,6 @@ pub async fn load_config_layers_state( } }; let project_layers = load_project_layers( - fs, &cwd, &project_trust_context.project_root, &project_trust_context, @@ -325,23 +369,22 @@ pub async fn load_config_layers_state( /// - If there is an error reading the file or parsing the TOML, returns an /// error. async fn load_config_toml_for_required_layer( - fs: &dyn ExecutorFileSystem, - toml_file: &AbsolutePathBuf, + toml_file: &ExecutorPathRef<'_>, create_entry: impl FnOnce(TomlValue) -> ConfigLayerEntry, ) -> io::Result { - let toml_value = match fs.read_file_text(toml_file, /*sandbox*/ None).await { + let toml_value = match toml_file.unsandboxed().read_file_text().await { Ok(contents) => { let config: TomlValue = toml::from_str(&contents).map_err(|err| { let config_error = - config_error_from_toml(toml_file.as_path(), &contents, err.clone()); + config_error_from_toml(toml_file.path().as_path(), &contents, err.clone()); io_error_from_config_error(io::ErrorKind::InvalidData, config_error, Some(err)) })?; - let config_parent = toml_file.as_path().parent().ok_or_else(|| { + let config_parent = toml_file.path().as_path().parent().ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidData, format!( "Config file {} has no parent directory", - toml_file.as_path().display() + toml_file.display() ), ) })?; @@ -353,10 +396,7 @@ async fn load_config_toml_for_required_layer( } else { Err(io::Error::new( e.kind(), - format!( - "Failed to read config file {}: {e}", - toml_file.as_path().display() - ), + format!("Failed to read config file {}: {e}", toml_file.display()), )) } } @@ -369,14 +409,10 @@ async fn load_config_toml_for_required_layer( /// `requirements.toml` location to `config_requirements_toml` by filling in /// any unset fields. async fn load_requirements_toml( - fs: &dyn ExecutorFileSystem, config_requirements_toml: &mut ConfigRequirementsWithSources, - requirements_toml_file: &AbsolutePathBuf, + requirements_toml_file: &ExecutorPathRef<'_>, ) -> io::Result<()> { - match fs - .read_file_text(requirements_toml_file, /*sandbox*/ None) - .await - { + match requirements_toml_file.unsandboxed().read_file_text().await { Ok(contents) => { let requirements_config: ConfigRequirementsToml = toml::from_str(&contents).map_err(|e| { @@ -384,13 +420,13 @@ async fn load_requirements_toml( io::ErrorKind::InvalidData, format!( "Error parsing requirements file {}: {e}", - requirements_toml_file.as_path().display(), + requirements_toml_file.display(), ), ) })?; config_requirements_toml.merge_unset_fields( RequirementSource::SystemRequirementsToml { - file: requirements_toml_file.clone(), + file: requirements_toml_file.path().clone(), }, requirements_config, ); @@ -401,7 +437,7 @@ async fn load_requirements_toml( e.kind(), format!( "Failed to read requirements file {}: {e}", - requirements_toml_file.as_path().display(), + requirements_toml_file.display(), ), )); } @@ -572,7 +608,7 @@ impl ProjectTrustDecision { impl ProjectTrustContext { fn decision_for_dir(&self, dir: &AbsolutePathBuf) -> ProjectTrustDecision { - let dir_key = project_trust_key(dir.as_path()); + let dir_key = executor_project_trust_key(dir.as_path()); if let Some(trust_level) = self.projects_trust.get(&dir_key).copied() { return ProjectTrustDecision { trust_level: Some(trust_level), @@ -643,9 +679,8 @@ fn project_layer_entry( } async fn project_trust_context( - fs: &dyn ExecutorFileSystem, merged_config: &TomlValue, - cwd: &AbsolutePathBuf, + cwd: &ExecutorPathRef<'_>, project_root_markers: &[String], config_base_dir: &Path, user_config_file: &AbsolutePathBuf, @@ -658,21 +693,21 @@ async fn project_trust_context( .map_err(|err| std::io::Error::new(std::io::ErrorKind::InvalidData, err))? }; - let project_root = find_project_root(fs, cwd, project_root_markers).await?; + let project_root = find_project_root(cwd, project_root_markers).await?; let projects = project_trust_config.projects.unwrap_or_default(); - let project_root_key = project_trust_key(project_root.as_path()); - let repo_root = resolve_root_git_project_for_trust(fs, cwd).await; + let project_root_key = executor_project_trust_key(project_root.as_path()); + let repo_root = resolve_root_git_project_for_trust_at(cwd).await; let repo_root_key = repo_root .as_ref() - .map(|root| project_trust_key(root.as_path())); + .map(|root| executor_project_trust_key(root.as_path())); let projects_trust = projects .into_iter() .filter_map(|(key, project)| { project .trust_level - .map(|trust_level| (project_trust_key(Path::new(&key)), trust_level)) + .map(|trust_level| (executor_project_trust_key(Path::new(&key)), trust_level)) }) .collect(); @@ -695,6 +730,10 @@ pub fn project_trust_key(project_path: &Path) -> String { .to_string() } +fn executor_project_trust_key(project_path: &Path) -> String { + project_path.to_string_lossy().to_string() +} + /// Takes a `toml::Value` parsed from a config.toml file and walks through it, /// resolving any `AbsolutePathBuf` fields against `base_dir`, returning a new /// `toml::Value` with the same shape but with paths resolved. @@ -756,27 +795,29 @@ fn copy_shape_from_original(original: &TomlValue, resolved: &TomlValue) -> TomlV } async fn find_project_root( - fs: &dyn ExecutorFileSystem, - cwd: &AbsolutePathBuf, + cwd: &ExecutorPathRef<'_>, project_root_markers: &[String], ) -> io::Result { if project_root_markers.is_empty() { - return Ok(cwd.clone()); + return Ok(cwd.path().clone()); } for ancestor in cwd.ancestors() { for marker in project_root_markers { let marker_path = ancestor.join(marker); - if fs - .get_metadata(&marker_path, /*sandbox*/ None) - .await - .is_ok() - { - return Ok(ancestor); + match marker_path.unsandboxed().exists().await { + Ok(true) => return Ok(ancestor.path().clone()), + Ok(false) => {} + Err(err) => { + tracing::warn!( + "failed to stat project root marker {}: {err:#}", + marker_path.display() + ); + } } } } - Ok(cwd.clone()) + Ok(cwd.path().clone()) } /// Return the appropriate list of layers (each with @@ -785,22 +826,19 @@ async fn find_project_root( /// starting from folders closest to `project_root` (which is the lowest /// precedence) to those closest to `cwd` (which is the highest precedence). async fn load_project_layers( - fs: &dyn ExecutorFileSystem, - cwd: &AbsolutePathBuf, + cwd: &ExecutorPathRef<'_>, project_root: &AbsolutePathBuf, trust_context: &ProjectTrustContext, codex_home: &Path, ) -> io::Result> { let codex_home_abs = AbsolutePathBuf::from_absolute_path(codex_home)?; - let codex_home_normalized = - normalize_path(codex_home_abs.as_path()).unwrap_or_else(|_| codex_home_abs.to_path_buf()); let mut dirs = cwd .ancestors() .scan(false, |done, a| { if *done { None } else { - if &a == project_root { + if a.path() == project_root { *done = true; } Some(a) @@ -811,30 +849,31 @@ async fn load_project_layers( let mut layers = Vec::new(); for dir in dirs { - let dot_codex_abs = dir.join(".codex"); - if !fs - .get_metadata(&dot_codex_abs, /*sandbox*/ None) - .await - .map(|metadata| metadata.is_directory) - .unwrap_or(false) - { - continue; + let dot_codex = dir.join(".codex"); + match dot_codex.unsandboxed().is_dir().await { + Ok(true) => {} + Ok(false) => continue, + Err(err) => { + tracing::warn!( + "failed to stat project config dir {}: {err:#}", + dot_codex.display() + ); + continue; + } } - let decision = trust_context.decision_for_dir(&dir); - let dot_codex_normalized = - normalize_path(dot_codex_abs.as_path()).unwrap_or_else(|_| dot_codex_abs.to_path_buf()); - if dot_codex_abs == codex_home_abs || dot_codex_normalized == codex_home_normalized { + let decision = trust_context.decision_for_dir(dir.path()); + if dot_codex.path() == &codex_home_abs { continue; } - let config_file = dot_codex_abs.join(CONFIG_TOML_FILE); - match fs.read_file_text(&config_file, /*sandbox*/ None).await { + let config_file = dot_codex.join(CONFIG_TOML_FILE); + match config_file.unsandboxed().read_file_text().await { Ok(contents) => { let config: TomlValue = match toml::from_str(&contents) { Ok(config) => config, Err(e) => { if decision.is_trusted() { - let config_file_display = config_file.as_path().display(); + let config_file_display = config_file.display(); return Err(io::Error::new( io::ErrorKind::InvalidData, format!( @@ -844,8 +883,8 @@ async fn load_project_layers( } layers.push(project_layer_entry( trust_context, - &dot_codex_abs, - &dir, + dot_codex.path(), + dir.path(), TomlValue::Table(toml::map::Map::new()), /*config_toml_exists*/ true, )); @@ -853,11 +892,11 @@ async fn load_project_layers( } }; let config = - resolve_relative_paths_in_config_toml(config, dot_codex_abs.as_path())?; + resolve_relative_paths_in_config_toml(config, dot_codex.path().as_path())?; let entry = project_layer_entry( trust_context, - &dot_codex_abs, - &dir, + dot_codex.path(), + dir.path(), config, /*config_toml_exists*/ true, ); @@ -870,13 +909,13 @@ async fn load_project_layers( // that are significant in the overall ConfigLayerStack. layers.push(project_layer_entry( trust_context, - &dot_codex_abs, - &dir, + dot_codex.path(), + dir.path(), TomlValue::Table(toml::map::Map::new()), /*config_toml_exists*/ false, )); } else { - let config_file_display = config_file.as_path().display(); + let config_file_display = config_file.display(); return Err(io::Error::new( err.kind(), format!("Failed to read project config file {config_file_display}: {err}"), diff --git a/codex-rs/core/src/config_loader/tests.rs b/codex-rs/core/src/config_loader/tests.rs index c706e91062..73da555d9e 100644 --- a/codex-rs/core/src/config_loader/tests.rs +++ b/codex-rs/core/src/config_loader/tests.rs @@ -16,6 +16,7 @@ use crate::config_loader::version_for_toml; use codex_config::CONFIG_TOML_FILE; use codex_config::config_toml::ConfigToml; use codex_config::config_toml::ProjectConfig; +use codex_exec_server::ExecutorPathRef; use codex_exec_server::LOCAL_FS; use codex_protocol::config_types::TrustLevel; use codex_protocol::config_types::WebSearchMode; @@ -538,13 +539,9 @@ personality = true .await?; let requirements_file = AbsolutePathBuf::try_from(requirements_file)?; + let requirements_file = ExecutorPathRef::new(LOCAL_FS.as_ref(), requirements_file); let mut config_requirements_toml = ConfigRequirementsWithSources::default(); - load_requirements_toml( - LOCAL_FS.as_ref(), - &mut config_requirements_toml, - &requirements_file, - ) - .await?; + load_requirements_toml(&mut config_requirements_toml, &requirements_file).await?; assert_eq!( config_requirements_toml @@ -706,12 +703,11 @@ allowed_approval_policies = ["on-request"] guardian_policy_config: None, }, ); - load_requirements_toml( + let requirements_file = ExecutorPathRef::new( LOCAL_FS.as_ref(), - &mut config_requirements_toml, - &AbsolutePathBuf::try_from(requirements_file)?, - ) - .await?; + AbsolutePathBuf::try_from(requirements_file)?, + ); + load_requirements_toml(&mut config_requirements_toml, &requirements_file).await?; assert_eq!( config_requirements_toml diff --git a/codex-rs/core/src/tools/handlers/view_image.rs b/codex-rs/core/src/tools/handlers/view_image.rs index 33ce2054ac..a1592cf031 100644 --- a/codex-rs/core/src/tools/handlers/view_image.rs +++ b/codex-rs/core/src/tools/handlers/view_image.rs @@ -1,3 +1,4 @@ +use codex_exec_server::ExecutorPath; use codex_protocol::models::FunctionCallOutputBody; use codex_protocol::models::FunctionCallOutputContentItem; use codex_protocol::models::FunctionCallOutputPayload; @@ -95,35 +96,29 @@ impl ToolHandler for ViewImageHandler { let sandbox = environment .is_remote() .then(|| turn.file_system_sandbox_context(/*additional_permissions*/ None)); + let image_path = ExecutorPath::new(environment.get_filesystem(), abs_path); + let image_file = image_path.with_sandbox(sandbox.as_ref()); - let metadata = environment - .get_filesystem() - .get_metadata(&abs_path, sandbox.as_ref()) - .await - .map_err(|error| { - FunctionCallError::RespondToModel(format!( - "unable to locate image at `{}`: {error}", - abs_path.display() - )) - })?; + let metadata = image_file.get_metadata().await.map_err(|error| { + FunctionCallError::RespondToModel(format!( + "unable to locate image at `{}`: {error}", + image_path.display() + )) + })?; if !metadata.is_file { return Err(FunctionCallError::RespondToModel(format!( "image path `{}` is not a file", - abs_path.display() + image_path.display() ))); } - let file_bytes = environment - .get_filesystem() - .read_file(&abs_path, sandbox.as_ref()) - .await - .map_err(|error| { - FunctionCallError::RespondToModel(format!( - "unable to read image at `{}`: {error}", - abs_path.display() - )) - })?; - let event_path = abs_path.clone(); + let file_bytes = image_file.read_file().await.map_err(|error| { + FunctionCallError::RespondToModel(format!( + "unable to read image at `{}`: {error}", + image_path.display() + )) + })?; + let event_path = image_path.path().clone(); let can_request_original_detail = can_request_original_image_detail(&turn.model_info); let use_original_detail = @@ -135,11 +130,11 @@ impl ToolHandler for ViewImageHandler { }; let image_detail = use_original_detail.then_some(ImageDetail::Original); - let image = - load_for_prompt_bytes(abs_path.as_path(), file_bytes, image_mode).map_err(|error| { + let image = load_for_prompt_bytes(image_path.path().as_path(), file_bytes, image_mode) + .map_err(|error| { FunctionCallError::RespondToModel(format!( "unable to process image at `{}`: {error}", - abs_path.display() + image_path.display() )) })?; let image_url = image.into_data_url(); diff --git a/codex-rs/exec-server/src/file_system.rs b/codex-rs/exec-server/src/file_system.rs index b09347686a..c250a649ed 100644 --- a/codex-rs/exec-server/src/file_system.rs +++ b/codex-rs/exec-server/src/file_system.rs @@ -3,6 +3,11 @@ use codex_protocol::config_types::WindowsSandboxLevel; use codex_protocol::models::PermissionProfile; use codex_protocol::protocol::SandboxPolicy; use codex_utils_absolute_path::AbsolutePathBuf; +use std::fmt; +use std::path::Display; +use std::path::Path; +use std::path::PathBuf; +use std::sync::Arc; use tokio::io; #[derive(Clone, Copy, Debug, Eq, PartialEq)] @@ -70,6 +75,397 @@ impl FileSystemSandboxContext { pub type FileSystemResult = io::Result; +/// A single filesystem operation mode for an executor-bound path. +/// +/// Construct this from [`ExecutorPath::unsandboxed`], +/// [`ExecutorPath::with_sandbox`], [`ExecutorPathRef::unsandboxed`], or +/// [`ExecutorPathRef::with_sandbox`] so call sites make their sandbox intent +/// visible at the operation boundary. +pub struct ExecutorPathAccess<'a> { + file_system: &'a dyn ExecutorFileSystem, + path: &'a AbsolutePathBuf, + sandbox: Option<&'a FileSystemSandboxContext>, +} + +impl<'a> ExecutorPathAccess<'a> { + pub fn new( + file_system: &'a dyn ExecutorFileSystem, + path: &'a AbsolutePathBuf, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> Self { + Self { + file_system, + path, + sandbox, + } + } + + pub fn path(&self) -> &AbsolutePathBuf { + self.path + } + + pub fn display(&self) -> Display<'_> { + self.path.display() + } + + pub async fn read_file(&self) -> FileSystemResult> { + self.file_system.read_file(self.path, self.sandbox).await + } + + pub async fn read_file_text(&self) -> FileSystemResult { + self.file_system + .read_file_text(self.path, self.sandbox) + .await + } + + pub async fn write_file(&self, contents: Vec) -> FileSystemResult<()> { + self.file_system + .write_file(self.path, contents, self.sandbox) + .await + } + + pub async fn create_directory( + &self, + create_directory_options: CreateDirectoryOptions, + ) -> FileSystemResult<()> { + self.file_system + .create_directory(self.path, create_directory_options, self.sandbox) + .await + } + + pub async fn get_metadata(&self) -> FileSystemResult { + self.file_system.get_metadata(self.path, self.sandbox).await + } + + pub async fn metadata_if_exists(&self) -> FileSystemResult> { + metadata_if_exists(self.file_system, self.path, self.sandbox).await + } + + pub async fn exists(&self) -> FileSystemResult { + Ok(self.metadata_if_exists().await?.is_some()) + } + + pub async fn is_dir(&self) -> FileSystemResult { + Ok(self + .metadata_if_exists() + .await? + .is_some_and(|metadata| metadata.is_directory)) + } + + pub async fn is_file(&self) -> FileSystemResult { + Ok(self + .metadata_if_exists() + .await? + .is_some_and(|metadata| metadata.is_file)) + } + + pub async fn read_directory(&self) -> FileSystemResult> { + self.file_system + .read_directory(self.path, self.sandbox) + .await + } + + pub async fn remove(&self, remove_options: RemoveOptions) -> FileSystemResult<()> { + self.file_system + .remove(self.path, remove_options, self.sandbox) + .await + } +} + +/// An absolute path bound to the executor filesystem where it should be +/// resolved. +/// +/// Use this when a path names a file on a specific executor. Keeping the +/// filesystem and path together avoids call sites that accidentally read a +/// remote/executor path through local process filesystem APIs. +#[derive(Clone)] +pub struct ExecutorPath { + file_system: Arc, + path: AbsolutePathBuf, +} + +impl ExecutorPath { + pub fn new(file_system: Arc, path: AbsolutePathBuf) -> Self { + Self { file_system, path } + } + + pub fn as_ref(&self) -> ExecutorPathRef<'_> { + ExecutorPathRef::new(self.file_system.as_ref(), self.path.clone()) + } + + pub fn path(&self) -> &AbsolutePathBuf { + &self.path + } + + pub fn to_path_buf(&self) -> PathBuf { + self.path.to_path_buf() + } + + pub fn display(&self) -> Display<'_> { + self.path.display() + } + + pub fn is_same_file_system(&self, file_system: &Arc) -> bool { + Arc::ptr_eq(&self.file_system, file_system) + } + + pub fn into_path(self) -> AbsolutePathBuf { + self.path + } + + pub fn with_path(&self, path: AbsolutePathBuf) -> Self { + Self { + file_system: Arc::clone(&self.file_system), + path, + } + } + + pub fn join>(&self, path: P) -> Self { + self.with_path(self.path.join(path)) + } + + pub fn parent(&self) -> Option { + self.path.parent().map(|path| self.with_path(path)) + } + + pub fn ancestors(&self) -> impl Iterator + '_ { + self.path.ancestors().map(|path| self.with_path(path)) + } + + pub fn unsandboxed(&self) -> ExecutorPathAccess<'_> { + self.with_sandbox(None) + } + + pub fn sandboxed<'a>( + &'a self, + sandbox: &'a FileSystemSandboxContext, + ) -> ExecutorPathAccess<'a> { + self.with_sandbox(Some(sandbox)) + } + + pub fn with_sandbox<'a>( + &'a self, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorPathAccess<'a> { + ExecutorPathAccess::new(self.file_system.as_ref(), &self.path, sandbox) + } + + pub async fn read_file( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.with_sandbox(sandbox).read_file().await + } + + pub async fn read_file_text( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + self.with_sandbox(sandbox).read_file_text().await + } + + pub async fn get_metadata( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + self.with_sandbox(sandbox).get_metadata().await + } + + pub async fn metadata_if_exists( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.with_sandbox(sandbox).metadata_if_exists().await + } + + pub async fn exists( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + Ok(self.metadata_if_exists(sandbox).await?.is_some()) + } + + pub async fn is_dir( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + Ok(self + .metadata_if_exists(sandbox) + .await? + .is_some_and(|metadata| metadata.is_directory)) + } + + pub async fn is_file( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + Ok(self + .metadata_if_exists(sandbox) + .await? + .is_some_and(|metadata| metadata.is_file)) + } + + pub async fn read_directory( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.with_sandbox(sandbox).read_directory().await + } +} + +impl fmt::Debug for ExecutorPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ExecutorPath") + .field("path", &self.path) + .finish_non_exhaustive() + } +} + +/// Borrowed filesystem plus owned absolute path for short-lived executor path +/// operations. +#[derive(Clone)] +pub struct ExecutorPathRef<'a> { + file_system: &'a dyn ExecutorFileSystem, + path: AbsolutePathBuf, +} + +impl<'a> ExecutorPathRef<'a> { + pub fn new(file_system: &'a dyn ExecutorFileSystem, path: AbsolutePathBuf) -> Self { + Self { file_system, path } + } + + pub fn path(&self) -> &AbsolutePathBuf { + &self.path + } + + pub fn to_path_buf(&self) -> PathBuf { + self.path.to_path_buf() + } + + pub fn display(&self) -> Display<'_> { + self.path.display() + } + + pub fn with_path(&self, path: AbsolutePathBuf) -> Self { + Self { + file_system: self.file_system, + path, + } + } + + pub fn join>(&self, path: P) -> Self { + self.with_path(self.path.join(path)) + } + + pub fn parent(&self) -> Option { + self.path.parent().map(|path| self.with_path(path)) + } + + pub fn ancestors(&self) -> impl Iterator + '_ { + self.path.ancestors().map(|path| self.with_path(path)) + } + + pub fn unsandboxed(&self) -> ExecutorPathAccess<'_> { + self.with_sandbox(None) + } + + pub fn sandboxed<'b>( + &'b self, + sandbox: &'b FileSystemSandboxContext, + ) -> ExecutorPathAccess<'b> { + self.with_sandbox(Some(sandbox)) + } + + pub fn with_sandbox<'b>( + &'b self, + sandbox: Option<&'b FileSystemSandboxContext>, + ) -> ExecutorPathAccess<'b> { + ExecutorPathAccess::new(self.file_system, &self.path, sandbox) + } + + pub async fn read_file( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.with_sandbox(sandbox).read_file().await + } + + pub async fn read_file_text( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + self.with_sandbox(sandbox).read_file_text().await + } + + pub async fn get_metadata( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + self.with_sandbox(sandbox).get_metadata().await + } + + pub async fn metadata_if_exists( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.with_sandbox(sandbox).metadata_if_exists().await + } + + pub async fn exists( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + Ok(self.metadata_if_exists(sandbox).await?.is_some()) + } + + pub async fn is_dir( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + Ok(self + .metadata_if_exists(sandbox) + .await? + .is_some_and(|metadata| metadata.is_directory)) + } + + pub async fn is_file( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult { + Ok(self + .metadata_if_exists(sandbox) + .await? + .is_some_and(|metadata| metadata.is_file)) + } + + pub async fn read_directory( + &self, + sandbox: Option<&FileSystemSandboxContext>, + ) -> FileSystemResult> { + self.with_sandbox(sandbox).read_directory().await + } +} + +impl fmt::Debug for ExecutorPathRef<'_> { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("ExecutorPathRef") + .field("path", &self.path) + .finish_non_exhaustive() + } +} + +async fn metadata_if_exists( + file_system: &dyn ExecutorFileSystem, + path: &AbsolutePathBuf, + sandbox: Option<&FileSystemSandboxContext>, +) -> FileSystemResult> { + match file_system.get_metadata(path, sandbox).await { + Ok(metadata) => Ok(Some(metadata)), + Err(err) if err.kind() == io::ErrorKind::NotFound => Ok(None), + Err(err) => Err(err), + } +} + #[async_trait] pub trait ExecutorFileSystem: Send + Sync { async fn read_file( diff --git a/codex-rs/exec-server/src/lib.rs b/codex-rs/exec-server/src/lib.rs index a73182697a..db0a5f1a40 100644 --- a/codex-rs/exec-server/src/lib.rs +++ b/codex-rs/exec-server/src/lib.rs @@ -28,6 +28,9 @@ pub use environment::EnvironmentManager; pub use file_system::CopyOptions; pub use file_system::CreateDirectoryOptions; pub use file_system::ExecutorFileSystem; +pub use file_system::ExecutorPath; +pub use file_system::ExecutorPathAccess; +pub use file_system::ExecutorPathRef; pub use file_system::FileMetadata; pub use file_system::FileSystemResult; pub use file_system::FileSystemSandboxContext; diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index e7642a8658..47a58030b9 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -5,6 +5,7 @@ use std::path::Path; use std::path::PathBuf; use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorPathRef; use codex_utils_absolute_path::AbsolutePathBuf; use futures::future::join_all; use schemars::JsonSchema; @@ -623,27 +624,31 @@ pub async fn resolve_root_git_project_for_trust( fs: &dyn ExecutorFileSystem, cwd: &AbsolutePathBuf, ) -> Option { - let base = match fs.get_metadata(cwd, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_directory => cwd.clone(), + resolve_root_git_project_for_trust_at(&ExecutorPathRef::new(fs, cwd.clone())).await +} + +/// Resolve the git trust root for a cwd that is already bound to an executor +/// filesystem. +pub async fn resolve_root_git_project_for_trust_at( + cwd: &ExecutorPathRef<'_>, +) -> Option { + let base = match cwd.unsandboxed().is_dir().await { + Ok(true) => cwd.clone(), _ => cwd.parent()?, }; - let (repo_root, dot_git) = find_ancestor_git_entry_with_fs(fs, &base).await?; - if fs - .get_metadata(&dot_git, /*sandbox*/ None) - .await - .ok()? - .is_directory - { - return Some(repo_root); + let (repo_root, dot_git) = find_ancestor_git_entry_with_fs(&base).await?; + if dot_git.unsandboxed().is_dir().await.ok()? { + return Some(repo_root.path().clone()); } - let git_dir_s = fs.read_file_text(&dot_git, /*sandbox*/ None).await.ok()?; + let git_dir_s = dot_git.unsandboxed().read_file_text().await.ok()?; let git_dir_rel = git_dir_s.trim().strip_prefix("gitdir:")?.trim(); if git_dir_rel.is_empty() { return None; } - let git_dir_path = AbsolutePathBuf::resolve_path_against_base(git_dir_rel, repo_root.as_path()); + let git_dir_path = + AbsolutePathBuf::resolve_path_against_base(git_dir_rel, repo_root.path().as_path()); let worktrees_dir = git_dir_path.parent()?; if worktrees_dir.as_path().file_name() != Some(OsStr::new("worktrees")) { return None; @@ -672,14 +677,14 @@ fn find_ancestor_git_entry(base_dir: &Path) -> Option<(PathBuf, PathBuf)> { None } -async fn find_ancestor_git_entry_with_fs( - fs: &dyn ExecutorFileSystem, - base_dir: &AbsolutePathBuf, -) -> Option<(AbsolutePathBuf, AbsolutePathBuf)> { +async fn find_ancestor_git_entry_with_fs<'a>( + base_dir: &ExecutorPathRef<'a>, +) -> Option<(ExecutorPathRef<'a>, ExecutorPathRef<'a>)> { for dir in base_dir.ancestors() { let dot_git = dir.join(".git"); - if fs.get_metadata(&dot_git, /*sandbox*/ None).await.is_ok() { - return Some((dir, dot_git)); + match dot_git.unsandboxed().exists().await { + Ok(true) => return Some((dir, dot_git)), + Ok(false) | Err(_) => {} } } None diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index d9d61d8650..1a41650669 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -43,4 +43,5 @@ pub use info::git_diff_to_remote; pub use info::local_git_branches; pub use info::recent_commits; pub use info::resolve_root_git_project_for_trust; +pub use info::resolve_root_git_project_for_trust_at; pub use platform::create_symlink; diff --git a/codex-rs/utils/plugins/src/plugin_namespace.rs b/codex-rs/utils/plugins/src/plugin_namespace.rs index adb709c7f0..a32acf5919 100644 --- a/codex-rs/utils/plugins/src/plugin_namespace.rs +++ b/codex-rs/utils/plugins/src/plugin_namespace.rs @@ -1,7 +1,6 @@ //! Resolve plugin namespace from skill file paths by walking ancestors for `plugin.json`. -use codex_exec_server::ExecutorFileSystem; -use codex_utils_absolute_path::AbsolutePathBuf; +use codex_exec_server::ExecutorPathRef; /// Relative path from a plugin root to its manifest file. pub const PLUGIN_MANIFEST_PATH: &str = ".codex-plugin/plugin.json"; @@ -13,22 +12,17 @@ struct RawPluginManifestName { name: String, } -async fn plugin_manifest_name( - fs: &dyn ExecutorFileSystem, - plugin_root: &AbsolutePathBuf, -) -> Option { +async fn plugin_manifest_name(plugin_root: &ExecutorPathRef<'_>) -> Option { let manifest_path = plugin_root.join(PLUGIN_MANIFEST_PATH); - match fs.get_metadata(&manifest_path, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_file => {} - Ok(_) | Err(_) => return None, + match manifest_path.unsandboxed().is_file().await { + Ok(true) => {} + Ok(false) | Err(_) => return None, } - let contents = fs - .read_file_text(&manifest_path, /*sandbox*/ None) - .await - .ok()?; + let contents = manifest_path.unsandboxed().read_file_text().await.ok()?; let RawPluginManifestName { name: raw_name } = serde_json::from_str(&contents).ok()?; Some( plugin_root + .path() .file_name() .and_then(|entry| entry.to_str()) .filter(|_| raw_name.trim().is_empty()) @@ -39,12 +33,9 @@ async fn plugin_manifest_name( /// Returns the plugin manifest `name` for the nearest ancestor of `path` that contains a valid /// plugin manifest (same `name` rules as full manifest loading in codex-core). -pub async fn plugin_namespace_for_skill_path( - fs: &dyn ExecutorFileSystem, - path: &AbsolutePathBuf, -) -> Option { +pub async fn plugin_namespace_for_skill_path(path: &ExecutorPathRef<'_>) -> Option { for ancestor in path.ancestors() { - if let Some(name) = plugin_manifest_name(fs, &ancestor).await { + if let Some(name) = plugin_manifest_name(&ancestor).await { return Some(name); } } @@ -54,6 +45,7 @@ pub async fn plugin_namespace_for_skill_path( #[cfg(test)] mod tests { use super::plugin_namespace_for_skill_path; + use codex_exec_server::ExecutorPathRef; use codex_exec_server::LOCAL_FS; use codex_utils_absolute_path::test_support::PathBufExt; use std::fs; @@ -75,7 +67,11 @@ mod tests { fs::write(&skill_path, "---\ndescription: search\n---\n").expect("write skill"); assert_eq!( - plugin_namespace_for_skill_path(LOCAL_FS.as_ref(), &skill_path.abs()).await, + plugin_namespace_for_skill_path(&ExecutorPathRef::new( + LOCAL_FS.as_ref(), + skill_path.abs(), + )) + .await, Some("sample".to_string()) ); }