mirror of
https://github.com/openai/codex.git
synced 2026-10-31 19:28:49 +00:00
perf(core): avoid duplicate first-turn filesystem discovery
This commit is contained in:
Generated
+1
@@ -2815,6 +2815,7 @@ dependencies = [
|
||||
"codex-config",
|
||||
"codex-context-fragments",
|
||||
"codex-exec-server",
|
||||
"codex-file-system",
|
||||
"codex-login",
|
||||
"codex-model-provider",
|
||||
"codex-otel",
|
||||
|
||||
@@ -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 }
|
||||
|
||||
@@ -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<Arc<dyn ExecutorFileSystem>>,
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
cwd: &AbsolutePathBuf,
|
||||
project_root: Option<(&AbsolutePathBuf, &AbsolutePathBuf)>,
|
||||
plugin_skill_roots: Vec<PluginSkillRoot>,
|
||||
extra_skill_roots: Vec<AbsolutePathBuf>,
|
||||
) -> Vec<SkillRoot> {
|
||||
@@ -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<Arc<dyn ExecutorFileSystem>>,
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
cwd: &AbsolutePathBuf,
|
||||
project_root: Option<(&AbsolutePathBuf, &AbsolutePathBuf)>,
|
||||
home_dir: Option<&AbsolutePathBuf>,
|
||||
plugin_skill_roots: Vec<PluginSkillRoot>,
|
||||
extra_skill_roots: Vec<AbsolutePathBuf>,
|
||||
@@ -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<Arc<dyn ExecutorFileSystem>>,
|
||||
config_layer_stack: &ConfigLayerStack,
|
||||
cwd: &AbsolutePathBuf,
|
||||
project_root: Option<(&AbsolutePathBuf, &AbsolutePathBuf)>,
|
||||
) -> Vec<SkillRoot> {
|
||||
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(),
|
||||
|
||||
@@ -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<Vec<PathUri>>,
|
||||
}
|
||||
|
||||
impl MetadataRecordingFileSystem {
|
||||
fn new() -> Self {
|
||||
Self {
|
||||
metadata_paths: Mutex::new(Vec::new()),
|
||||
}
|
||||
}
|
||||
|
||||
fn metadata_paths(&self) -> Vec<PathUri> {
|
||||
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<u8>> {
|
||||
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<u8>,
|
||||
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<ReadDirectoryEntry>> {
|
||||
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(),
|
||||
)
|
||||
|
||||
@@ -33,9 +33,16 @@ pub struct SkillsLoadInput {
|
||||
pub effective_skill_roots: Vec<PluginSkillRoot>,
|
||||
pub config_layer_stack: ConfigLayerStack,
|
||||
pub bundled_skills_enabled: bool,
|
||||
project_root: Option<ProjectRootHint>,
|
||||
plugin_skill_snapshots: Option<PluginSkillSnapshots>,
|
||||
}
|
||||
|
||||
#[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(),
|
||||
)
|
||||
|
||||
+107
-40
@@ -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<UserInstructions>,
|
||||
environments: &TurnEnvironmentSnapshot,
|
||||
) -> Option<LoadedAgentsMd> {
|
||||
load_project_instructions_with_roots(config, user_instructions, environments)
|
||||
.await
|
||||
.loaded
|
||||
}
|
||||
|
||||
fn effective_project_root_markers(config: &Config) -> Vec<String> {
|
||||
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<LoadedAgentsMd>,
|
||||
pub(crate) project_roots: HashMap<String, PathUri>,
|
||||
}
|
||||
|
||||
/// 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<UserInstructions>,
|
||||
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<LoadedAgentsMd>,
|
||||
project_root: Option<PathUri>,
|
||||
}
|
||||
|
||||
/// 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<Option<LoadedAgentsMd>> {
|
||||
) -> io::Result<ReadAgentsMdOutcome> {
|
||||
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<Option<LoadedAgentsMd>> {
|
||||
Ok(read_agents_md_with_root(config, fs, environment_id, cwd)
|
||||
.await?
|
||||
.loaded)
|
||||
}
|
||||
|
||||
struct AgentsMdPathDiscovery {
|
||||
paths: Vec<PathUri>,
|
||||
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<Vec<PathUri>> {
|
||||
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<AgentsMdPathDiscovery> {
|
||||
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> {
|
||||
|
||||
@@ -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<PathUri> {
|
||||
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<Arc<LoadedAgentsMd>> {
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -443,14 +443,21 @@ async fn warm_plugins_and_skills_for_session_init(
|
||||
plugins_manager: Arc<PluginsManager>,
|
||||
skills_service: Arc<SkillsService>,
|
||||
turn_environments: &TurnEnvironmentSnapshot,
|
||||
project_root: Option<(AbsolutePathBuf, AbsolutePathBuf)>,
|
||||
) -> Vec<SkillError> {
|
||||
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",
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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<String, PathBuf>,
|
||||
baseline_by_path: HashMap<TrackedPath, TrackedContent>,
|
||||
current_by_path: HashMap<TrackedPath, TrackedContent>,
|
||||
@@ -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<Item = (String, PathBuf)>,
|
||||
) -> 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<Item = (String, PathBuf)>,
|
||||
) {
|
||||
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;
|
||||
|
||||
@@ -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");
|
||||
|
||||
Reference in New Issue
Block a user