diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 0d4d3c33bf..bf1a53db76 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2815,6 +2815,7 @@ dependencies = [ "codex-config", "codex-context-fragments", "codex-exec-server", + "codex-file-system", "codex-login", "codex-model-provider", "codex-otel", diff --git a/codex-rs/core-skills/Cargo.toml b/codex-rs/core-skills/Cargo.toml index f86ecdf75b..b0c83eab16 100644 --- a/codex-rs/core-skills/Cargo.toml +++ b/codex-rs/core-skills/Cargo.toml @@ -18,6 +18,7 @@ codex-analytics = { workspace = true } codex-config = { workspace = true } codex-context-fragments = { workspace = true } codex-exec-server = { workspace = true } +codex-file-system = { workspace = true } codex-login = { workspace = true } codex-model-provider = { workspace = true } codex-otel = { workspace = true } diff --git a/codex-rs/core-skills/src/loader.rs b/codex-rs/core-skills/src/loader.rs index c9ace54204..3093586c4b 100644 --- a/codex-rs/core-skills/src/loader.rs +++ b/codex-rs/core-skills/src/loader.rs @@ -20,6 +20,8 @@ use codex_config::merge_toml_values; use codex_config::project_root_markers_from_config; use codex_exec_server::ExecutorFileSystem; use codex_exec_server::LOCAL_FS; +use codex_file_system::FindUpErrorPolicy; +use codex_file_system::find_nearest_native_ancestor_with_markers; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; @@ -238,6 +240,7 @@ pub(crate) async fn skill_roots( fs: Option>, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, + project_root: Option<(&AbsolutePathBuf, &AbsolutePathBuf)>, plugin_skill_roots: Vec, extra_skill_roots: Vec, ) -> Vec { @@ -247,6 +250,7 @@ pub(crate) async fn skill_roots( fs, config_layer_stack, cwd, + project_root, home_dir.as_ref(), plugin_skill_roots, extra_skill_roots, @@ -258,6 +262,7 @@ async fn skill_roots_with_home_dir( fs: Option>, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, + project_root: Option<(&AbsolutePathBuf, &AbsolutePathBuf)>, home_dir: Option<&AbsolutePathBuf>, plugin_skill_roots: Vec, extra_skill_roots: Vec, @@ -279,7 +284,7 @@ async fn skill_roots_with_home_dir( plugin_namespace: None, plugin_root: None, })); - roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd).await); + roots.extend(repo_agents_skill_roots(fs, config_layer_stack, cwd, project_root).await); dedupe_skill_roots_by_path(&mut roots); roots } @@ -374,12 +379,20 @@ async fn repo_agents_skill_roots( fs: Option>, config_layer_stack: &ConfigLayerStack, cwd: &AbsolutePathBuf, + project_root: Option<(&AbsolutePathBuf, &AbsolutePathBuf)>, ) -> Vec { let Some(fs) = fs 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 = match project_root.filter(|(discovery_cwd, root)| { + *discovery_cwd == cwd && cwd.as_path().starts_with(root.as_path()) + }) { + Some((_, project_root)) => project_root.clone(), + None => { + let project_root_markers = project_root_markers_from_stack(config_layer_stack); + find_project_root(fs.as_ref(), 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 { @@ -438,24 +451,17 @@ async fn find_project_root( return cwd.clone(); } - for ancestor in cwd.ancestors() { - for marker in project_root_markers { - let marker_path = ancestor.join(marker); - let marker_path_uri = PathUri::from_abs_path(&marker_path); - match fs.get_metadata(&marker_path_uri, /*sandbox*/ None).await { - Ok(_) => return ancestor, - Err(err) if err.kind() == io::ErrorKind::NotFound => {} - Err(err) => { - tracing::warn!( - "failed to stat project root marker {}: {err:#}", - marker_path.display() - ); - } - } - } - } - - cwd.clone() + find_nearest_native_ancestor_with_markers( + fs, + cwd, + project_root_markers.to_vec(), + FindUpErrorPolicy::Ignore, + /*sandbox*/ None, + ) + .await + .ok() + .flatten() + .unwrap_or_else(|| cwd.clone()) } fn dirs_between_project_root_and_cwd( @@ -1280,6 +1286,7 @@ pub(crate) async fn skill_roots_from_layer_stack( Some(fs), config_layer_stack, cwd, + /*project_root*/ None, home_dir, Vec::new(), Vec::new(), diff --git a/codex-rs/core-skills/src/loader_tests.rs b/codex-rs/core-skills/src/loader_tests.rs index 8cd371c9a7..83243e2634 100644 --- a/codex-rs/core-skills/src/loader_tests.rs +++ b/codex-rs/core-skills/src/loader_tests.rs @@ -4,18 +4,29 @@ use codex_config::ConfigLayerEntry; use codex_config::ConfigLayerStack; use codex_config::ConfigRequirements; use codex_config::ConfigRequirementsToml; +use codex_exec_server::CopyOptions; +use codex_exec_server::CreateDirectoryOptions; +use codex_exec_server::ExecutorFileSystem; +use codex_exec_server::ExecutorFileSystemFuture; +use codex_exec_server::FileMetadata; +use codex_exec_server::FileSystemReadStream; +use codex_exec_server::FileSystemSandboxContext; use codex_exec_server::LOCAL_FS; +use codex_exec_server::ReadDirectoryEntry; +use codex_exec_server::RemoveOptions; use codex_protocol::protocol::Product; use codex_protocol::protocol::SkillScope; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_absolute_path::test_support::PathBufExt; use codex_utils_absolute_path::test_support::PathExt; +use codex_utils_path_uri::PathUri; use dunce::canonicalize as canonicalize_path; use pretty_assertions::assert_eq; use std::fs; use std::path::Path; use std::path::PathBuf; use std::sync::Arc; +use std::sync::Mutex; use tempfile::TempDir; use toml::Value as TomlValue; @@ -26,6 +37,108 @@ struct TestConfig { config_layer_stack: ConfigLayerStack, } +struct MetadataRecordingFileSystem { + metadata_paths: Mutex>, +} + +impl MetadataRecordingFileSystem { + fn new() -> Self { + Self { + metadata_paths: Mutex::new(Vec::new()), + } + } + + fn metadata_paths(&self) -> Vec { + self.metadata_paths + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .clone() + } +} + +impl ExecutorFileSystem for MetadataRecordingFileSystem { + fn canonicalize<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, PathUri> { + LOCAL_FS.canonicalize(path, sandbox) + } + + fn read_file<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + LOCAL_FS.read_file(path, sandbox) + } + + fn read_file_stream<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileSystemReadStream> { + LOCAL_FS.read_file_stream(path, sandbox) + } + + fn write_file<'a>( + &'a self, + path: &'a PathUri, + contents: Vec, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + LOCAL_FS.write_file(path, contents, sandbox) + } + + fn create_directory<'a>( + &'a self, + path: &'a PathUri, + options: CreateDirectoryOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + LOCAL_FS.create_directory(path, options, sandbox) + } + + fn get_metadata<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, FileMetadata> { + self.metadata_paths + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + .push(path.clone()); + LOCAL_FS.get_metadata(path, sandbox) + } + + fn read_directory<'a>( + &'a self, + path: &'a PathUri, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, Vec> { + LOCAL_FS.read_directory(path, sandbox) + } + + fn remove<'a>( + &'a self, + path: &'a PathUri, + options: RemoveOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + LOCAL_FS.remove(path, options, sandbox) + } + + fn copy<'a>( + &'a self, + source_path: &'a PathUri, + destination_path: &'a PathUri, + options: CopyOptions, + sandbox: Option<&'a FileSystemSandboxContext>, + ) -> ExecutorFileSystemFuture<'a, ()> { + LOCAL_FS.copy(source_path, destination_path, options, sandbox) + } +} + async fn make_config(codex_home: &TempDir) -> TestConfig { make_config_for_cwd(codex_home, codex_home.path().to_path_buf()).await } @@ -145,6 +258,94 @@ fn normalized(path: &Path) -> AbsolutePathBuf { .abs() } +#[tokio::test] +async fn repo_skill_roots_use_one_shot_hint_then_observe_new_nearest_marker() { + let codex_home = tempfile::tempdir().expect("codex home"); + let repo = codex_home.path().join("repo"); + let cwd = repo.join("packages/app"); + let repo_skills = repo.join(".agents/skills"); + fs::create_dir_all(&cwd).expect("create cwd"); + fs::create_dir_all(&repo_skills).expect("create repo skills"); + mark_as_git_repo(&repo); + let config = make_config_for_cwd(&codex_home, cwd).await; + let repo = repo.abs(); + + let supplied_root_fs = Arc::new(MetadataRecordingFileSystem::new()); + let roots = super::repo_agents_skill_roots( + Some(supplied_root_fs.clone()), + &config.config_layer_stack, + &config.cwd, + Some((&config.cwd, &repo)), + ) + .await; + assert!(roots.iter().any(|root| root.path == repo_skills.abs())); + assert!( + supplied_root_fs + .metadata_paths() + .iter() + .all(|path| path.basename().as_deref() != Some(".git")), + "a supplied root must avoid marker probes" + ); + + let nested_root = codex_home.path().join("repo/packages"); + let nested_skills = nested_root.join(".agents/skills"); + fs::create_dir_all(&nested_skills).expect("create nested repo skills"); + mark_as_git_repo(&nested_root); + + let fallback_fs = Arc::new(MetadataRecordingFileSystem::new()); + let roots = super::repo_agents_skill_roots( + Some(fallback_fs.clone()), + &config.config_layer_stack, + &config.cwd, + /*project_root*/ None, + ) + .await; + assert!(roots.iter().any(|root| root.path == nested_skills.abs())); + assert!(roots.iter().all(|root| root.path != repo_skills.abs())); + assert!( + fallback_fs + .metadata_paths() + .iter() + .any(|path| path == &PathUri::from_abs_path(&nested_root.join(".git").abs())), + "the post-startup fallback must observe a newly-created nearer marker" + ); +} + +#[tokio::test] +async fn repo_skill_roots_ignore_hint_discovered_for_different_cwd() { + let codex_home = tempfile::tempdir().expect("codex home"); + let repo = codex_home.path().join("repo"); + let nested_root = repo.join("packages"); + let cwd = nested_root.join("app"); + let repo_skills = repo.join(".agents/skills"); + let nested_skills = nested_root.join(".agents/skills"); + fs::create_dir_all(&cwd).expect("create cwd"); + fs::create_dir_all(&repo_skills).expect("create repo skills"); + fs::create_dir_all(&nested_skills).expect("create nested repo skills"); + mark_as_git_repo(&repo); + mark_as_git_repo(&nested_root); + let config = make_config_for_cwd(&codex_home, cwd).await; + let repo = repo.abs(); + + let fs = Arc::new(MetadataRecordingFileSystem::new()); + let roots = super::repo_agents_skill_roots( + Some(fs.clone()), + &config.config_layer_stack, + &config.cwd, + Some((&repo, &repo)), + ) + .await; + + assert!(roots.iter().any(|root| root.path == nested_skills.abs())); + assert!(roots.iter().all(|root| root.path != repo_skills.abs())); + assert!( + fs.metadata_paths() + .iter() + .any(|path| path == &PathUri::from_abs_path(&nested_root.join(".git").abs())), + "a hint from a different cwd must be ignored so a nearer marker can be found" + ); +} + #[tokio::test] async fn skill_roots_from_layer_stack_maps_user_to_user_and_system_cache_and_system_to_admin() -> anyhow::Result<()> { @@ -2147,6 +2348,7 @@ async fn skill_roots_include_admin_with_lowest_priority() { Some(Arc::clone(&LOCAL_FS)), &cfg.config_layer_stack, &cfg.cwd, + /*project_root*/ None, Vec::new(), Vec::new(), ) diff --git a/codex-rs/core-skills/src/service.rs b/codex-rs/core-skills/src/service.rs index b4e0d7396c..2688607d14 100644 --- a/codex-rs/core-skills/src/service.rs +++ b/codex-rs/core-skills/src/service.rs @@ -33,9 +33,16 @@ pub struct SkillsLoadInput { pub effective_skill_roots: Vec, pub config_layer_stack: ConfigLayerStack, pub bundled_skills_enabled: bool, + project_root: Option, plugin_skill_snapshots: Option, } +#[derive(Debug, Clone)] +struct ProjectRootHint { + discovery_cwd: AbsolutePathBuf, + project_root: AbsolutePathBuf, +} + impl SkillsLoadInput { pub fn new( cwd: AbsolutePathBuf, @@ -48,10 +55,25 @@ impl SkillsLoadInput { effective_skill_roots, config_layer_stack, bundled_skills_enabled, + project_root: None, plugin_skill_snapshots: None, } } + /// Reuses a project root discovered immediately before this load from the same config and cwd. + /// Callers must not retain this hint across turns because filesystem markers can change. + pub fn with_project_root( + mut self, + discovery_cwd: AbsolutePathBuf, + project_root: AbsolutePathBuf, + ) -> Self { + self.project_root = Some(ProjectRootHint { + discovery_cwd, + project_root, + }); + self + } + /// Attaches plugin skill snapshots parsed during plugin loading, when available. pub fn with_plugin_skill_snapshots( mut self, @@ -156,6 +178,10 @@ impl SkillsService { fs, &input.config_layer_stack, &input.cwd, + input + .project_root + .as_ref() + .map(|hint| (&hint.discovery_cwd, &hint.project_root)), input.effective_skill_roots.clone(), self.extra_roots(), ) @@ -184,6 +210,10 @@ impl SkillsService { fs.clone(), &input.config_layer_stack, &input.cwd, + input + .project_root + .as_ref() + .map(|hint| (&hint.discovery_cwd, &hint.project_root)), input.effective_skill_roots.clone(), self.extra_roots(), ) diff --git a/codex-rs/core/src/agents_md.rs b/codex-rs/core/src/agents_md.rs index 1c01c8873f..520085add7 100644 --- a/codex-rs/core/src/agents_md.rs +++ b/codex-rs/core/src/agents_md.rs @@ -29,6 +29,7 @@ use codex_file_system::FindUpErrorPolicy; use codex_file_system::find_nearest_ancestor_with_markers; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; +use std::collections::HashMap; use std::io; use toml::Value as TomlValue; use tracing::error; @@ -44,15 +45,57 @@ const AGENTS_MD_SEPARATOR: &str = "\n\n--- project-doc ---\n\n"; /// Loads project AGENTS.md content and combines it with host-provided user /// instructions. +#[cfg(test)] pub(crate) async fn load_project_instructions( config: &Config, user_instructions: Option, environments: &TurnEnvironmentSnapshot, ) -> Option { + load_project_instructions_with_roots(config, user_instructions, environments) + .await + .loaded +} + +fn effective_project_root_markers(config: &Config) -> Vec { + let mut merged = TomlValue::Table(toml::map::Map::new()); + for layer in config.config_layer_stack.get_layers( + ConfigLayerStackOrdering::LowestPrecedenceFirst, + /*include_disabled*/ false, + ) { + if matches!(layer.name, ConfigLayerSource::Project { .. }) { + continue; + } + merge_toml_values(&mut merged, &layer.config); + } + match project_root_markers_from_config(&merged) { + Ok(Some(markers)) => markers, + Ok(None) => default_project_root_markers(), + Err(err) => { + tracing::warn!("invalid project_root_markers: {err}"); + default_project_root_markers() + } + } +} + +pub(crate) struct ProjectInstructionsLoadOutcome { + pub(crate) loaded: Option, + pub(crate) project_roots: HashMap, +} + +/// Loads project instructions and retains the project roots found along the way. +/// +/// Skills discovery uses the same project-root markers. Keeping this result avoids repeating the +/// remote marker walk immediately after AGENTS.md discovery during session startup. +pub(crate) async fn load_project_instructions_with_roots( + config: &Config, + user_instructions: Option, + environments: &TurnEnvironmentSnapshot, +) -> ProjectInstructionsLoadOutcome { let mut loaded = LoadedAgentsMd::from_user_instructions(user_instructions); + let mut project_roots = HashMap::new(); for turn_environment in &environments.turn_environments { let filesystem = turn_environment.environment.get_filesystem(); - match read_agents_md( + match read_agents_md_with_root( config, filesystem.as_ref(), &turn_environment.environment_id, @@ -60,8 +103,14 @@ pub(crate) async fn load_project_instructions( ) .await { - Ok(Some(docs)) => loaded.entries.extend(docs.entries), - Ok(None) => {} + Ok(outcome) => { + if let Some(project_root) = outcome.project_root { + project_roots.insert(turn_environment.environment_id.clone(), project_root); + } + if let Some(docs) = outcome.loaded { + loaded.entries.extend(docs.entries); + } + } Err(e) => { error!( environment_id = turn_environment.environment_id, @@ -71,7 +120,15 @@ pub(crate) async fn load_project_instructions( } } - (!loaded.is_empty()).then_some(loaded) + ProjectInstructionsLoadOutcome { + loaded: (!loaded.is_empty()).then_some(loaded), + project_roots, + } +} + +struct ReadAgentsMdOutcome { + loaded: Option, + project_root: Option, } /// Attempt to locate and load AGENTS.md documentation. @@ -80,27 +137,27 @@ pub(crate) async fn load_project_instructions( /// discovered doc. 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( +async fn read_agents_md_with_root( config: &Config, fs: &dyn ExecutorFileSystem, environment_id: &str, cwd: &PathUri, -) -> io::Result> { +) -> io::Result { let max_total = config.project_doc_max_bytes; if max_total == 0 { - return Ok(None); + return Ok(ReadAgentsMdOutcome { + loaded: None, + project_root: None, + }); } - let paths = agents_md_paths(config, cwd, fs).await?; - if paths.is_empty() { - return Ok(None); - } + let discovery = discover_agents_md_paths(config, cwd, fs).await?; let mut remaining: u64 = max_total as u64; let mut loaded = LoadedAgentsMd::default(); - for p in paths { + for p in discovery.paths { if remaining == 0 { break; } @@ -137,40 +194,48 @@ async fn read_agents_md( } } - if loaded.is_empty() { - Ok(None) - } else { - Ok(Some(loaded)) - } + Ok(ReadAgentsMdOutcome { + loaded: (!loaded.is_empty()).then_some(loaded), + project_root: Some(discovery.project_root), + }) +} + +#[cfg(test)] +async fn read_agents_md( + config: &Config, + fs: &dyn ExecutorFileSystem, + environment_id: &str, + cwd: &PathUri, +) -> io::Result> { + Ok(read_agents_md_with_root(config, fs, environment_id, cwd) + .await? + .loaded) +} + +struct AgentsMdPathDiscovery { + paths: Vec, + project_root: PathUri, } /// Discovers AGENTS.md files from the project root to the current working /// directory, inclusive. Symlinks are allowed. +#[cfg(test)] async fn agents_md_paths( config: &Config, cwd: &PathUri, fs: &dyn ExecutorFileSystem, ) -> io::Result> { + Ok(discover_agents_md_paths(config, cwd, fs).await?.paths) +} + +async fn discover_agents_md_paths( + config: &Config, + cwd: &PathUri, + fs: &dyn ExecutorFileSystem, +) -> io::Result { let dir = cwd.clone(); - let mut merged = TomlValue::Table(toml::map::Map::new()); - for layer in config.config_layer_stack.get_layers( - ConfigLayerStackOrdering::LowestPrecedenceFirst, - /*include_disabled*/ false, - ) { - if matches!(layer.name, ConfigLayerSource::Project { .. }) { - continue; - } - merge_toml_values(&mut merged, &layer.config); - } - let project_root_markers = match project_root_markers_from_config(&merged) { - Ok(Some(markers)) => markers, - Ok(None) => default_project_root_markers(), - Err(err) => { - tracing::warn!("invalid project_root_markers: {err}"); - default_project_root_markers() - } - }; + let project_root_markers = effective_project_root_markers(config); let project_root = find_nearest_ancestor_with_markers( fs, &dir, @@ -179,12 +244,13 @@ async fn agents_md_paths( /*sandbox*/ None, ) .await?; - let search_dirs = if let Some(root) = project_root { + let project_root = project_root.unwrap_or_else(|| dir.clone()); + let search_dirs = { let mut dirs = Vec::new(); let mut cursor = dir.clone(); loop { dirs.push(cursor.clone()); - if cursor == root { + if cursor == project_root { break; } let Some(parent) = cursor.parent() else { @@ -194,8 +260,6 @@ async fn agents_md_paths( } dirs.reverse(); dirs - } else { - vec![dir] }; let mut found = Vec::new(); @@ -216,7 +280,10 @@ async fn agents_md_paths( } } } - Ok(found) + Ok(AgentsMdPathDiscovery { + paths: found, + project_root, + }) } fn candidate_filenames(config: &Config) -> Vec<&str> { diff --git a/codex-rs/core/src/agents_md_manager.rs b/codex-rs/core/src/agents_md_manager.rs index 1e3a3f291d..5252f6a856 100644 --- a/codex-rs/core/src/agents_md_manager.rs +++ b/codex-rs/core/src/agents_md_manager.rs @@ -1,9 +1,10 @@ use crate::agents_md::LoadedAgentsMd; -use crate::agents_md::load_project_instructions; +use crate::agents_md::load_project_instructions_with_roots; use crate::config::Config; use crate::environment_selection::TurnEnvironmentSnapshot; use codex_extension_api::UserInstructions; use codex_protocol::protocol::TurnEnvironmentSelection; +use codex_utils_path_uri::PathUri; use std::sync::Arc; use tokio::sync::Mutex; @@ -28,19 +29,29 @@ impl AgentsMdManager { } } - pub(crate) async fn refresh(&self, config: &Config, environments: &TurnEnvironmentSnapshot) { + pub(crate) async fn refresh( + &self, + config: &Config, + environments: &TurnEnvironmentSnapshot, + ) -> Option { let selections = environments.to_selections(); if self.cache.lock().await.selections.as_ref() == Some(&selections) { - return; + return None; } - let loaded = - load_project_instructions(config, self.user_instructions.clone(), environments) - .await - .map(Arc::new); + let mut outcome = load_project_instructions_with_roots( + config, + self.user_instructions.clone(), + environments, + ) + .await; + let primary_project_root = environments + .primary() + .and_then(|primary| outcome.project_roots.remove(&primary.environment_id)); let mut cache = self.cache.lock().await; cache.selections = Some(selections); - cache.loaded = loaded; + cache.loaded = outcome.loaded.map(Arc::new); + primary_project_root } pub(crate) async fn get_loaded(&self) -> Option> { diff --git a/codex-rs/core/src/session/mod.rs b/codex-rs/core/src/session/mod.rs index fa240b890b..be5c9ad1a9 100644 --- a/codex-rs/core/src/session/mod.rs +++ b/codex-rs/core/src/session/mod.rs @@ -2873,7 +2873,8 @@ impl Session { turn_context.environments.clone() }; if deferred_executor_enabled { - self.services + let _ = self + .services .agents_md_manager .refresh(&turn_context.config, &environments) .await; diff --git a/codex-rs/core/src/session/session.rs b/codex-rs/core/src/session/session.rs index 08abb46377..cef91a5e03 100644 --- a/codex-rs/core/src/session/session.rs +++ b/codex-rs/core/src/session/session.rs @@ -443,14 +443,21 @@ async fn warm_plugins_and_skills_for_session_init( plugins_manager: Arc, skills_service: Arc, turn_environments: &TurnEnvironmentSnapshot, + project_root: Option<(AbsolutePathBuf, AbsolutePathBuf)>, ) -> Vec { let fs = turn_environments.primary_filesystem(); let plugins_input = config.plugins_config_input(); let plugin_outcome = plugins_manager.plugins_for_config(&plugins_input).await; let effective_skill_roots = plugin_outcome.effective_plugin_skill_roots(); let plugin_skill_snapshots = plugins_manager.plugin_skill_snapshots_for_config(&plugins_input); - let skills_input = skills_load_input_from_config(config.as_ref(), effective_skill_roots) - .with_plugin_skill_snapshots(plugin_skill_snapshots); + let skills_input = skills_load_input_from_config(config.as_ref(), effective_skill_roots); + let skills_input = match project_root { + Some((discovery_cwd, project_root)) => { + skills_input.with_project_root(discovery_cwd, project_root) + } + None => skills_input, + } + .with_plugin_skill_snapshots(plugin_skill_snapshots); skills_service .snapshot_for_config(&skills_input, fs) .await @@ -900,14 +907,20 @@ impl Session { turn_environments.update_selections(session_configuration.environment_selections()); let resolved_environments = turn_environments.snapshot().await; let agents_md_manager = Arc::new(AgentsMdManager::new(user_instructions)); - agents_md_manager + let project_root = agents_md_manager .refresh(config.as_ref(), &resolved_environments) - .await; + .await + .and_then(|root| root.to_abs_path().ok()); + let project_root = resolved_environments + .primary() + .and_then(|environment| environment.cwd().to_abs_path().ok()) + .zip(project_root); let plugin_skill_errors = warm_plugins_and_skills_for_session_init( Arc::clone(&config), Arc::clone(&plugins_manager), Arc::clone(&skills_service), &resolved_environments, + project_root, ) .instrument(info_span!( "session_init.plugin_skill_warmup", diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index 708f42f0ff..46087eab66 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -167,10 +167,9 @@ pub(crate) async fn run_turn( // run_turn owns the step used to seed context and make the first sampling request. let first_step_context = sess.capture_step_context(Arc::clone(&turn_context)).await; // Keep the exact model-visible state used by this turn and its inline compactions. - let (mut world_state, display_roots) = tokio::join!( - sess.record_context_updates_and_set_reference_context_item(first_step_context.as_ref()), - turn_diff_display_roots(turn_context.as_ref()), - ); + let mut world_state = sess + .record_context_updates_and_set_reference_context_item(first_step_context.as_ref()) + .await; let Some((injection_items, explicitly_enabled_connectors)) = build_skills_and_plugins( &sess, @@ -210,9 +209,7 @@ pub(crate) async fn run_turn( let mut stop_hook_active = false; // Although from the perspective of codex.rs, TurnDiffTracker has the lifecycle of a Task which contains // many turns, from the perspective of the user, it is a single turn. - let turn_diff_tracker = Arc::new(tokio::sync::Mutex::new( - TurnDiffTracker::with_environment_display_roots(display_roots), - )); + let turn_diff_tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())); // `ModelClientSession` is turn-scoped and caches WebSocket + sticky routing state, so we reuse // one instance across retries within this turn. @@ -459,7 +456,7 @@ pub(crate) async fn run_turn( } #[instrument(level = "trace", skip_all)] -async fn turn_diff_display_roots(turn_context: &TurnContext) -> Vec<(String, PathBuf)> { +pub(crate) async fn turn_diff_display_roots(turn_context: &TurnContext) -> Vec<(String, PathBuf)> { let mut display_roots = Vec::new(); for turn_environment in &turn_context.environments.turn_environments { // TODO(anp): Migrate git-root discovery and diff display roots to PathUri so foreign diff --git a/codex-rs/core/src/tools/events.rs b/codex-rs/core/src/tools/events.rs index 757125a03d..ed2571c4b0 100644 --- a/codex-rs/core/src/tools/events.rs +++ b/codex-rs/core/src/tools/events.rs @@ -1,5 +1,6 @@ use crate::function_tool::FunctionCallError; use crate::session::session::Session; +use crate::session::turn::turn_diff_display_roots; use crate::session::turn_context::TurnContext; use crate::tools::context::SharedTurnDiffTracker; use crate::tools::sandboxing::ToolError; @@ -83,7 +84,9 @@ fn tracker_update_for_known_delta<'a>( environment_id: Option<&str>, delta: &'a AppliedPatchDelta, ) -> TurnDiffTrackerUpdate<'a> { - if delta.is_exact() && delta.is_empty() { + if !delta.is_exact() { + TurnDiffTrackerUpdate::Invalidate + } else if delta.is_empty() { TurnDiffTrackerUpdate::None } else { TurnDiffTrackerUpdate::Track { @@ -590,6 +593,19 @@ async fn emit_patch_end( .await; if let Some(tracker) = ctx.turn_diff_tracker { + if matches!(&tracker_update, TurnDiffTrackerUpdate::Track { .. }) { + let should_discover_display_roots = { + let guard = tracker.lock().await; + guard.needs_display_roots() + }; + if should_discover_display_roots { + let display_roots = turn_diff_display_roots(ctx.turn).await; + let mut guard = tracker.lock().await; + if guard.needs_display_roots() { + guard.set_environment_display_roots(display_roots); + } + } + } let (should_emit_turn_diff, unified_diff) = { let mut guard = tracker.lock().await; let had_unified_diff = guard.has_unified_diff(); diff --git a/codex-rs/core/src/turn_diff_tracker.rs b/codex-rs/core/src/turn_diff_tracker.rs index ed8e9684ad..13ea1ec572 100644 --- a/codex-rs/core/src/turn_diff_tracker.rs +++ b/codex-rs/core/src/turn_diff_tracker.rs @@ -49,6 +49,7 @@ struct DiffCacheKey { /// mutations, without rereading the workspace filesystem. pub struct TurnDiffTracker { valid: bool, + display_roots_initialized: bool, display_roots_by_environment: HashMap, baseline_by_path: HashMap, current_by_path: HashMap, @@ -64,6 +65,7 @@ impl Default for TurnDiffTracker { fn default() -> Self { Self { valid: true, + display_roots_initialized: false, display_roots_by_environment: HashMap::new(), baseline_by_path: HashMap::new(), current_by_path: HashMap::new(), @@ -82,14 +84,27 @@ impl TurnDiffTracker { Self::default() } + #[cfg(test)] pub fn with_environment_display_roots( display_roots: impl IntoIterator, ) -> Self { let mut tracker = Self::new(); - tracker.display_roots_by_environment = display_roots.into_iter().collect(); + tracker.set_environment_display_roots(display_roots); tracker } + pub(crate) fn needs_display_roots(&self) -> bool { + self.valid && !self.display_roots_initialized + } + + pub(crate) fn set_environment_display_roots( + &mut self, + display_roots: impl IntoIterator, + ) { + self.display_roots_by_environment = display_roots.into_iter().collect(); + self.display_roots_initialized = true; + } + pub fn track_delta(&mut self, environment_id: &str, delta: &AppliedPatchDelta) { if !self.valid { return; diff --git a/codex-rs/core/src/turn_diff_tracker_tests.rs b/codex-rs/core/src/turn_diff_tracker_tests.rs index 5f984a2c4f..865ef79968 100644 --- a/codex-rs/core/src/turn_diff_tracker_tests.rs +++ b/codex-rs/core/src/turn_diff_tracker_tests.rs @@ -50,6 +50,22 @@ fn tracker_with_root(root: &Path) -> TurnDiffTracker { TurnDiffTracker::with_environment_display_roots([("".to_string(), root.to_path_buf())]) } +#[test] +fn display_roots_are_initialized_only_when_supplied() { + let dir = tempdir().expect("tempdir"); + let mut tracker = TurnDiffTracker::new(); + + assert!(tracker.needs_display_roots()); + + tracker.set_environment_display_roots([("local".to_string(), dir.path().to_path_buf())]); + + assert!(!tracker.needs_display_roots()); + + let mut invalid_tracker = TurnDiffTracker::new(); + invalid_tracker.invalidate(); + assert!(!invalid_tracker.needs_display_roots()); +} + #[tokio::test] async fn accumulates_add_then_update_as_single_add() { let dir = tempdir().expect("tempdir");