mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Make AGENTS.md discovery FS-aware (#15826)
## Summary - make AGENTS.md discovery and loading fully FS-aware and remove the non-FS discover helper - migrate remote-aware codex-core tests to use TestEnv workspace setup instead of syncing a local workspace copy - add AGENTS.md corner-case coverage, including directory fallbacks and remote-aware integration coverage ## Testing - cargo test -p codex-core project_doc -- --nocapture - cargo test -p codex-core hierarchical_agents -- --nocapture - cargo test -p codex-core agents_md -- --nocapture - cargo test -p codex-tui status -- --nocapture - cargo test -p codex-tui-app-server status -- --nocapture - just fix - just fmt - just bazel-lock-update - just bazel-lock-check - just argument-comment-lint - remote Linux executor tests in progress via scripts/test-remote-env.sh
This commit is contained in:
committed by
GitHub
Unverified
parent
232db0613a
commit
4bb507d2c4
@@ -536,7 +536,11 @@ impl Codex {
|
||||
config.startup_warnings.push(message);
|
||||
}
|
||||
|
||||
let user_instructions = get_user_instructions(&config).await;
|
||||
let environment = environment_manager
|
||||
.current()
|
||||
.await
|
||||
.map_err(|err| CodexErr::Fatal(format!("failed to create environment: {err}")))?;
|
||||
let user_instructions = get_user_instructions(&config, environment.as_deref()).await;
|
||||
|
||||
let exec_policy = if crate::guardian::is_guardian_reviewer_source(&session_source) {
|
||||
// Guardian review should rely on the built-in shell safety checks,
|
||||
@@ -664,12 +668,12 @@ impl Codex {
|
||||
agent_status_tx.clone(),
|
||||
conversation_history,
|
||||
session_source_clone,
|
||||
environment_manager,
|
||||
skills_manager,
|
||||
plugins_manager,
|
||||
mcp_manager.clone(),
|
||||
skills_watcher,
|
||||
agent_control,
|
||||
environment,
|
||||
)
|
||||
.await
|
||||
.map_err(|e| {
|
||||
@@ -1518,12 +1522,12 @@ impl Session {
|
||||
agent_status: watch::Sender<AgentStatus>,
|
||||
initial_history: InitialHistory,
|
||||
session_source: SessionSource,
|
||||
environment_manager: Arc<EnvironmentManager>,
|
||||
skills_manager: Arc<SkillsManager>,
|
||||
plugins_manager: Arc<PluginsManager>,
|
||||
mcp_manager: Arc<McpManager>,
|
||||
skills_watcher: Arc<SkillsWatcher>,
|
||||
agent_control: AgentControl,
|
||||
environment: Option<Arc<Environment>>,
|
||||
) -> anyhow::Result<Arc<Self>> {
|
||||
debug!(
|
||||
"Configuring session: model={}; provider={:?}",
|
||||
@@ -1963,7 +1967,7 @@ impl Session {
|
||||
code_mode_service: crate::tools::code_mode::CodeModeService::new(
|
||||
config.js_repl_node_path.clone(),
|
||||
),
|
||||
environment: environment_manager.current().await?,
|
||||
environment,
|
||||
};
|
||||
services
|
||||
.model_client
|
||||
|
||||
@@ -2612,14 +2612,16 @@ async fn session_new_fails_when_zsh_fork_enabled_without_zsh_path() {
|
||||
agent_status_tx,
|
||||
InitialHistory::New,
|
||||
SessionSource::Exec,
|
||||
Arc::new(codex_exec_server::EnvironmentManager::new(
|
||||
/*exec_server_url*/ None,
|
||||
)),
|
||||
skills_manager,
|
||||
plugins_manager,
|
||||
mcp_manager,
|
||||
Arc::new(SkillsWatcher::noop()),
|
||||
AgentControl::default(),
|
||||
Some(Arc::new(
|
||||
codex_exec_server::Environment::create(/*exec_server_url*/ None)
|
||||
.await
|
||||
.expect("create environment"),
|
||||
)),
|
||||
)
|
||||
.await;
|
||||
|
||||
|
||||
@@ -21,10 +21,12 @@ use crate::config_loader::default_project_root_markers;
|
||||
use crate::config_loader::merge_toml_values;
|
||||
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_features::Feature;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use dunce::canonicalize as normalize_path;
|
||||
use std::path::PathBuf;
|
||||
use tokio::io::AsyncReadExt;
|
||||
use std::io;
|
||||
use toml::Value as TomlValue;
|
||||
use tracing::error;
|
||||
|
||||
@@ -76,8 +78,19 @@ fn render_js_repl_instructions(config: &Config) -> Option<String> {
|
||||
|
||||
/// Combines `Config::instructions` and `AGENTS.md` (if present) into a single
|
||||
/// string of instructions.
|
||||
pub(crate) async fn get_user_instructions(config: &Config) -> Option<String> {
|
||||
let project_docs = read_project_docs(config).await;
|
||||
pub(crate) async fn get_user_instructions(
|
||||
config: &Config,
|
||||
environment: Option<&Environment>,
|
||||
) -> Option<String> {
|
||||
let fs = environment?.get_filesystem();
|
||||
get_user_instructions_with_fs(config, fs.as_ref()).await
|
||||
}
|
||||
|
||||
pub(crate) async fn get_user_instructions_with_fs(
|
||||
config: &Config,
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
) -> Option<String> {
|
||||
let project_docs = read_project_docs_with_fs(config, fs).await;
|
||||
|
||||
let mut output = String::new();
|
||||
|
||||
@@ -125,14 +138,25 @@ pub(crate) async fn get_user_instructions(config: &Config) -> Option<String> {
|
||||
/// 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.
|
||||
pub async fn read_project_docs(config: &Config) -> std::io::Result<Option<String>> {
|
||||
pub async fn read_project_docs(
|
||||
config: &Config,
|
||||
environment: &Environment,
|
||||
) -> io::Result<Option<String>> {
|
||||
let fs = environment.get_filesystem();
|
||||
read_project_docs_with_fs(config, fs.as_ref()).await
|
||||
}
|
||||
|
||||
async fn read_project_docs_with_fs(
|
||||
config: &Config,
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
) -> io::Result<Option<String>> {
|
||||
let max_total = config.project_doc_max_bytes;
|
||||
|
||||
if max_total == 0 {
|
||||
return Ok(None);
|
||||
}
|
||||
|
||||
let paths = discover_project_doc_paths(config)?;
|
||||
let paths = discover_project_doc_paths(config, fs).await?;
|
||||
if paths.is_empty() {
|
||||
return Ok(None);
|
||||
}
|
||||
@@ -145,16 +169,22 @@ pub async fn read_project_docs(config: &Config) -> std::io::Result<Option<String
|
||||
break;
|
||||
}
|
||||
|
||||
let file = match tokio::fs::File::open(&p).await {
|
||||
Ok(f) => f,
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue,
|
||||
Err(e) => return Err(e),
|
||||
};
|
||||
match fs.get_metadata(&p).await {
|
||||
Ok(metadata) if !metadata.is_file => continue,
|
||||
Ok(_) => {}
|
||||
Err(err) if err.kind() == io::ErrorKind::NotFound => continue,
|
||||
Err(err) => return Err(err),
|
||||
}
|
||||
|
||||
let size = file.metadata().await?.len();
|
||||
let mut reader = tokio::io::BufReader::new(file).take(remaining);
|
||||
let mut data: Vec<u8> = Vec::new();
|
||||
reader.read_to_end(&mut data).await?;
|
||||
let mut data = match fs.read_file(&p).await {
|
||||
Ok(data) => data,
|
||||
Err(err) if err.kind() == io::ErrorKind::NotFound => continue,
|
||||
Err(err) => return Err(err),
|
||||
};
|
||||
let size = data.len() as u64;
|
||||
if size > remaining {
|
||||
data.truncate(remaining as usize);
|
||||
}
|
||||
|
||||
if size > remaining {
|
||||
tracing::warn!(
|
||||
@@ -183,10 +213,17 @@ pub async fn read_project_docs(config: &Config) -> std::io::Result<Option<String
|
||||
/// 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.
|
||||
pub fn discover_project_doc_paths(config: &Config) -> std::io::Result<Vec<PathBuf>> {
|
||||
let mut dir = config.cwd.to_path_buf();
|
||||
pub async fn discover_project_doc_paths(
|
||||
config: &Config,
|
||||
fs: &dyn ExecutorFileSystem,
|
||||
) -> io::Result<Vec<AbsolutePathBuf>> {
|
||||
if config.project_doc_max_bytes == 0 {
|
||||
return Ok(Vec::new());
|
||||
}
|
||||
|
||||
let mut dir = config.cwd.clone();
|
||||
if let Ok(canon) = normalize_path(&dir) {
|
||||
dir = canon;
|
||||
dir = AbsolutePathBuf::try_from(canon)?;
|
||||
}
|
||||
|
||||
let mut merged = TomlValue::Table(toml::map::Map::new());
|
||||
@@ -211,14 +248,14 @@ pub fn discover_project_doc_paths(config: &Config) -> std::io::Result<Vec<PathBu
|
||||
if !project_root_markers.is_empty() {
|
||||
for ancestor in dir.ancestors() {
|
||||
for marker in &project_root_markers {
|
||||
let marker_path = ancestor.join(marker);
|
||||
let marker_exists = match std::fs::metadata(&marker_path) {
|
||||
let marker_path = AbsolutePathBuf::try_from(ancestor.join(marker))?;
|
||||
let marker_exists = match fs.get_metadata(&marker_path).await {
|
||||
Ok(_) => true,
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => false,
|
||||
Err(e) => return Err(e),
|
||||
Err(err) if err.kind() == io::ErrorKind::NotFound => false,
|
||||
Err(err) => return Err(err),
|
||||
};
|
||||
if marker_exists {
|
||||
project_root = Some(ancestor.to_path_buf());
|
||||
project_root = Some(AbsolutePathBuf::try_from(ancestor.to_path_buf())?);
|
||||
break;
|
||||
}
|
||||
}
|
||||
@@ -228,11 +265,11 @@ pub fn discover_project_doc_paths(config: &Config) -> std::io::Result<Vec<PathBu
|
||||
}
|
||||
}
|
||||
|
||||
let search_dirs: Vec<PathBuf> = if let Some(root) = project_root {
|
||||
let search_dirs: Vec<AbsolutePathBuf> = if let Some(root) = project_root {
|
||||
let mut dirs = Vec::new();
|
||||
let mut cursor = dir.as_path();
|
||||
let mut cursor = dir.clone();
|
||||
loop {
|
||||
dirs.push(cursor.to_path_buf());
|
||||
dirs.push(cursor.clone());
|
||||
if cursor == root {
|
||||
break;
|
||||
}
|
||||
@@ -247,29 +284,25 @@ pub fn discover_project_doc_paths(config: &Config) -> std::io::Result<Vec<PathBu
|
||||
vec![dir]
|
||||
};
|
||||
|
||||
let mut found: Vec<PathBuf> = Vec::new();
|
||||
let mut found: Vec<AbsolutePathBuf> = Vec::new();
|
||||
let candidate_filenames = candidate_filenames(config);
|
||||
for d in search_dirs {
|
||||
for name in &candidate_filenames {
|
||||
let candidate = d.join(name);
|
||||
match std::fs::symlink_metadata(&candidate) {
|
||||
Ok(md) => {
|
||||
let ft = md.file_type();
|
||||
// Allow regular files and symlinks; opening will later fail for dangling links.
|
||||
if ft.is_file() || ft.is_symlink() {
|
||||
found.push(candidate);
|
||||
break;
|
||||
}
|
||||
let candidate = d.join(name)?;
|
||||
match fs.get_metadata(&candidate).await {
|
||||
Ok(md) if md.is_file => {
|
||||
found.push(candidate);
|
||||
break;
|
||||
}
|
||||
Err(e) if e.kind() == std::io::ErrorKind::NotFound => continue,
|
||||
Err(e) => return Err(e),
|
||||
Ok(_) => {}
|
||||
Err(err) if err.kind() == io::ErrorKind::NotFound => continue,
|
||||
Err(err) => return Err(err),
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Ok(found)
|
||||
}
|
||||
|
||||
fn candidate_filenames<'a>(config: &'a Config) -> Vec<&'a str> {
|
||||
let mut names: Vec<&'a str> =
|
||||
Vec::with_capacity(2 + config.project_doc_fallback_filenames.len());
|
||||
|
||||
@@ -1,12 +1,23 @@
|
||||
use super::*;
|
||||
use crate::config::ConfigBuilder;
|
||||
use codex_exec_server::LOCAL_FS;
|
||||
use codex_features::Feature;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::TempDirExt;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::fs;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::TempDir;
|
||||
|
||||
async fn get_user_instructions(config: &Config) -> Option<String> {
|
||||
super::get_user_instructions_with_fs(config, LOCAL_FS.as_ref()).await
|
||||
}
|
||||
|
||||
async fn discover_project_doc_paths(config: &Config) -> std::io::Result<Vec<AbsolutePathBuf>> {
|
||||
super::discover_project_doc_paths(config, LOCAL_FS.as_ref()).await
|
||||
}
|
||||
|
||||
/// Helper that returns a `Config` pointing at `root` and using `limit` as
|
||||
/// the maximum number of bytes to embed from AGENTS.md. The caller can
|
||||
/// optionally specify a custom `instructions` string – when `None` the
|
||||
@@ -85,6 +96,16 @@ async fn no_doc_file_returns_none() {
|
||||
assert!(res.is_none(), "Expected None when AGENTS.md is absent");
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn no_environment_returns_none() {
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
let config = make_config(&tmp, /*limit*/ 4096, Some("user instructions")).await;
|
||||
|
||||
let res = super::get_user_instructions(&config, /*environment*/ None).await;
|
||||
|
||||
assert_eq!(res, None);
|
||||
}
|
||||
|
||||
/// Small file within the byte-limit is returned unmodified.
|
||||
#[tokio::test]
|
||||
async fn doc_smaller_than_limit_is_returned() {
|
||||
@@ -161,6 +182,18 @@ async fn zero_byte_limit_disables_docs() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn zero_byte_limit_disables_discovery() {
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
fs::write(tmp.path().join("AGENTS.md"), "something").unwrap();
|
||||
|
||||
let discovery =
|
||||
discover_project_doc_paths(&make_config(&tmp, /*limit*/ 0, /*instructions*/ None).await)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
assert_eq!(discovery, Vec::<AbsolutePathBuf>::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn js_repl_instructions_are_appended_when_enabled() {
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
@@ -293,11 +326,18 @@ async fn project_root_markers_are_honored_for_agents_discovery() {
|
||||
.await;
|
||||
cfg.cwd = nested.abs();
|
||||
|
||||
let discovery = discover_project_doc_paths(&cfg).expect("discover paths");
|
||||
let expected_parent =
|
||||
dunce::canonicalize(root.path().join("AGENTS.md")).expect("canonical parent doc path");
|
||||
let expected_child =
|
||||
dunce::canonicalize(cfg.cwd.as_path().join("AGENTS.md")).expect("canonical child doc path");
|
||||
let discovery = discover_project_doc_paths(&cfg)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
let expected_parent = AbsolutePathBuf::try_from(
|
||||
dunce::canonicalize(root.path().join("AGENTS.md")).expect("canonical parent doc path"),
|
||||
)
|
||||
.expect("absolute parent doc path");
|
||||
let expected_child = AbsolutePathBuf::try_from(
|
||||
dunce::canonicalize(cfg.cwd.join("AGENTS.md").expect("absolute child doc path"))
|
||||
.expect("canonical child doc path"),
|
||||
)
|
||||
.expect("absolute child doc path");
|
||||
assert_eq!(discovery.len(), 2);
|
||||
assert_eq!(discovery[0], expected_parent);
|
||||
assert_eq!(discovery[1], expected_child);
|
||||
@@ -321,7 +361,9 @@ async fn agents_local_md_preferred() {
|
||||
|
||||
assert_eq!(res, "local");
|
||||
|
||||
let discovery = discover_project_doc_paths(&cfg).expect("discover paths");
|
||||
let discovery = discover_project_doc_paths(&cfg)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
assert_eq!(discovery.len(), 1);
|
||||
assert_eq!(
|
||||
discovery[0].file_name().unwrap().to_string_lossy(),
|
||||
@@ -371,7 +413,9 @@ async fn agents_md_preferred_over_fallbacks() {
|
||||
|
||||
assert_eq!(res, "primary");
|
||||
|
||||
let discovery = discover_project_doc_paths(&cfg).expect("discover paths");
|
||||
let discovery = discover_project_doc_paths(&cfg)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
assert_eq!(discovery.len(), 1);
|
||||
assert!(
|
||||
discovery[0]
|
||||
@@ -382,6 +426,73 @@ async fn agents_md_preferred_over_fallbacks() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn agents_md_directory_is_ignored() {
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
fs::create_dir(tmp.path().join("AGENTS.md")).unwrap();
|
||||
|
||||
let cfg = make_config(&tmp, /*limit*/ 4096, /*instructions*/ None).await;
|
||||
|
||||
let res = get_user_instructions(&cfg).await;
|
||||
assert_eq!(res, None);
|
||||
|
||||
let discovery = discover_project_doc_paths(&cfg)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
assert_eq!(discovery, Vec::<AbsolutePathBuf>::new());
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn agents_md_special_file_is_ignored() {
|
||||
use std::ffi::CString;
|
||||
use std::os::unix::ffi::OsStrExt;
|
||||
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
let path = tmp.path().join("AGENTS.md");
|
||||
let c_path = CString::new(path.as_os_str().as_bytes()).expect("path without nul");
|
||||
// SAFETY: `c_path` is a valid, nul-terminated path and `mkfifo` does not
|
||||
// retain the pointer after the call.
|
||||
let rc = unsafe { libc::mkfifo(c_path.as_ptr(), 0o644) };
|
||||
assert_eq!(rc, 0);
|
||||
|
||||
let cfg = make_config(&tmp, /*limit*/ 4096, /*instructions*/ None).await;
|
||||
|
||||
let res = get_user_instructions(&cfg).await;
|
||||
assert_eq!(res, None);
|
||||
|
||||
let discovery = discover_project_doc_paths(&cfg)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
assert_eq!(discovery, Vec::<AbsolutePathBuf>::new());
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn override_directory_falls_back_to_agents_md_file() {
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
fs::create_dir(tmp.path().join(LOCAL_PROJECT_DOC_FILENAME)).unwrap();
|
||||
fs::write(tmp.path().join(DEFAULT_PROJECT_DOC_FILENAME), "primary").unwrap();
|
||||
|
||||
let cfg = make_config(&tmp, /*limit*/ 4096, /*instructions*/ None).await;
|
||||
|
||||
let res = get_user_instructions(&cfg)
|
||||
.await
|
||||
.expect("AGENTS.md should be used when override is a directory");
|
||||
assert_eq!(res, "primary");
|
||||
|
||||
let discovery = discover_project_doc_paths(&cfg)
|
||||
.await
|
||||
.expect("discover paths");
|
||||
assert_eq!(discovery.len(), 1);
|
||||
assert_eq!(
|
||||
discovery[0]
|
||||
.file_name()
|
||||
.expect("file name")
|
||||
.to_string_lossy(),
|
||||
DEFAULT_PROJECT_DOC_FILENAME
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn skills_are_not_appended_to_project_doc() {
|
||||
let tmp = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
Reference in New Issue
Block a user