mirror of
https://github.com/openai/codex.git
synced 2026-10-18 17:18:52 +00:00
Skip redirected external-agent migration destinations (#39221)
## Why Repository-scoped imports must not follow redirected Codex paths and write generated configuration outside the repository. ## What changed - Reject repository migration scopes when generated configuration paths are symlinks or, on Windows, reparse points. - Recheck hook and subagent destinations during import, skipping redirected directories, files, and dangling links without reporting them as imported. ## Testing Add service and app-server coverage for redirected generated destinations, including a repository path replaced with a symlink after detection. GitOrigin-RevId: 3afb521ead9d76250f0e967eccfdd1dc194f027a
This commit is contained in:
@@ -24,6 +24,8 @@ use codex_app_server_protocol::ExternalAgentConfigImportHistoriesReadResponse;
|
||||
use codex_app_server_protocol::ExternalAgentConfigImportHistoryRecordResponse;
|
||||
use codex_app_server_protocol::ExternalAgentConfigImportProgressNotification;
|
||||
use codex_app_server_protocol::ExternalAgentConfigImportResponse;
|
||||
#[cfg(unix)]
|
||||
use codex_app_server_protocol::ExternalAgentConfigImportTypeResult;
|
||||
use codex_app_server_protocol::ExternalAgentConfigMigrationItemType;
|
||||
use codex_app_server_protocol::ExternalAgentImportedConnectorCandidate;
|
||||
use codex_app_server_protocol::ExternalAgentImportedConnectorSource;
|
||||
@@ -141,6 +143,87 @@ async fn external_agent_config_detect_accepts_migration_source_and_defaults_unkn
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn external_agent_config_import_skips_repository_redirect_after_detection() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let repository = TempDir::new()?;
|
||||
let repo_root = repository.path();
|
||||
let repo_config_dir = repo_root.join(".codex");
|
||||
let global_config = codex_home.path().join("config.toml");
|
||||
std::fs::create_dir(repo_root.join(".git"))?;
|
||||
std::fs::create_dir(&repo_config_dir)?;
|
||||
std::fs::write(
|
||||
repo_root.join(".mcp.json"),
|
||||
r#"{"mcpServers":{"repository-server":{"command":"repository-server"}}}"#,
|
||||
)?;
|
||||
std::fs::write(&global_config, "model = \"gpt-5.4\"\n")?;
|
||||
|
||||
let home_dir = codex_home.path().display().to_string();
|
||||
let mut mcp = TestAppServer::builder()
|
||||
.with_codex_home(codex_home.path())
|
||||
.without_auto_env()
|
||||
.with_env_overrides(&[("HOME", Some(home_dir.as_str()))])
|
||||
.build_initialized_with_timeout(DEFAULT_TIMEOUT)
|
||||
.await?;
|
||||
let original_global_config = std::fs::read_to_string(&global_config)?;
|
||||
|
||||
let detect_request_id = mcp
|
||||
.send_raw_request(
|
||||
"externalAgentConfig/detect",
|
||||
Some(serde_json::json!({
|
||||
"includeHome": false,
|
||||
"cwds": [repo_root],
|
||||
})),
|
||||
)
|
||||
.await?;
|
||||
let detection: ExternalAgentConfigDetectResponse =
|
||||
timeout(DEFAULT_TIMEOUT, mcp.read_response(detect_request_id)).await??;
|
||||
assert_eq!(detection.items.len(), 1);
|
||||
assert_eq!(
|
||||
detection.items[0].item_type,
|
||||
ExternalAgentConfigMigrationItemType::McpServerConfig
|
||||
);
|
||||
|
||||
std::fs::remove_dir(&repo_config_dir)?;
|
||||
std::os::unix::fs::symlink(codex_home.path(), &repo_config_dir)?;
|
||||
|
||||
let import_request_id = mcp
|
||||
.send_raw_request(
|
||||
"externalAgentConfig/import",
|
||||
Some(serde_json::json!({
|
||||
"migrationItems": detection.items,
|
||||
})),
|
||||
)
|
||||
.await?;
|
||||
let response: ExternalAgentConfigImportResponse =
|
||||
timeout(DEFAULT_TIMEOUT, mcp.read_response(import_request_id)).await??;
|
||||
let import_id = assert_import_response(response);
|
||||
let completed: ExternalAgentConfigImportCompletedNotification = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_notification("externalAgentConfig/import/completed"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
assert_eq!(
|
||||
completed,
|
||||
ExternalAgentConfigImportCompletedNotification {
|
||||
import_id,
|
||||
item_type_results: vec![ExternalAgentConfigImportTypeResult {
|
||||
item_type: ExternalAgentConfigMigrationItemType::McpServerConfig,
|
||||
successes: Vec::new(),
|
||||
failures: Vec::new(),
|
||||
}],
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(global_config)?,
|
||||
original_global_config
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn external_agent_config_detect_does_not_block_configuration_reads() -> Result<()> {
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use crate::invalid_data_error;
|
||||
use crate::scope::is_redirected_destination;
|
||||
use crate::utils::is_missing_or_empty_text_file;
|
||||
use serde_json::Value as JsonValue;
|
||||
use std::fs;
|
||||
@@ -250,6 +251,9 @@ pub(super) fn copy_hook_scripts(
|
||||
}
|
||||
|
||||
fn copy_dir_recursive_skip_existing(source: &Path, target: &Path) -> io::Result<()> {
|
||||
if is_redirected_destination(target)? {
|
||||
return Ok(());
|
||||
}
|
||||
fs::create_dir_all(target)?;
|
||||
for entry in fs::read_dir(source)? {
|
||||
let entry = entry?;
|
||||
@@ -258,7 +262,10 @@ fn copy_dir_recursive_skip_existing(source: &Path, target: &Path) -> io::Result<
|
||||
let file_type = entry.file_type()?;
|
||||
if file_type.is_dir() {
|
||||
copy_dir_recursive_skip_existing(&source_path, &target_path)?;
|
||||
} else if file_type.is_file() && !target_path.exists() {
|
||||
} else if file_type.is_file()
|
||||
&& !target_path.exists()
|
||||
&& !is_redirected_destination(&target_path)?
|
||||
{
|
||||
fs::copy(source_path, target_path)?;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,7 +1,11 @@
|
||||
use std::fs;
|
||||
use std::io;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
|
||||
#[cfg(windows)]
|
||||
use std::os::windows::fs::MetadataExt;
|
||||
|
||||
/// The filesystem boundary within which migration detection or import runs.
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
pub(super) enum MigrationScope {
|
||||
@@ -40,14 +44,31 @@ impl MigrationScope {
|
||||
loop {
|
||||
let git_path = current.join(".git");
|
||||
if git_path.is_dir() || git_path.is_file() {
|
||||
return Ok(Some(Self::Repository { root: current }));
|
||||
return Self::repository(current);
|
||||
}
|
||||
if !current.pop() {
|
||||
break;
|
||||
}
|
||||
}
|
||||
|
||||
Ok(Some(Self::Repository { root: fallback }))
|
||||
Self::repository(fallback)
|
||||
}
|
||||
|
||||
fn repository(root: PathBuf) -> io::Result<Option<Self>> {
|
||||
for relative_path in [
|
||||
".codex",
|
||||
".codex/config.toml",
|
||||
".codex/agents",
|
||||
".codex/hooks",
|
||||
".agents",
|
||||
".agents/skills",
|
||||
] {
|
||||
if is_redirected_destination(&root.join(relative_path))? {
|
||||
return Ok(None);
|
||||
}
|
||||
}
|
||||
|
||||
Ok(Some(Self::Repository { root }))
|
||||
}
|
||||
|
||||
pub(super) fn repo_root(&self) -> Option<&Path> {
|
||||
@@ -66,6 +87,20 @@ impl MigrationScope {
|
||||
}
|
||||
}
|
||||
|
||||
pub(super) fn is_redirected_destination(path: &Path) -> io::Result<bool> {
|
||||
let metadata = match fs::symlink_metadata(path) {
|
||||
Ok(metadata) => metadata,
|
||||
Err(error) if error.kind() == io::ErrorKind::NotFound => return Ok(false),
|
||||
Err(error) => return Err(error),
|
||||
};
|
||||
|
||||
let is_redirected = metadata.file_type().is_symlink();
|
||||
#[cfg(windows)]
|
||||
let is_redirected = is_redirected || metadata.file_attributes() & 0x0400 != 0;
|
||||
|
||||
Ok(is_redirected)
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "scope_tests.rs"]
|
||||
mod tests;
|
||||
|
||||
@@ -1,6 +1,77 @@
|
||||
use super::super::*;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[cfg(unix)]
|
||||
#[tokio::test]
|
||||
async fn repo_migration_skips_redirected_generated_destinations() -> std::io::Result<()> {
|
||||
let root = TempDir::new()?;
|
||||
let repo = root.path().join("repo");
|
||||
let source = repo.join(EXTERNAL_AGENT_DIR);
|
||||
let target = repo.join(".codex");
|
||||
let outside = root.path().join("outside");
|
||||
fs::create_dir_all(repo.join(".git"))?;
|
||||
fs::create_dir_all(source.join("agents"))?;
|
||||
fs::create_dir_all(source.join("hooks/nested"))?;
|
||||
fs::create_dir_all(target.join("agents"))?;
|
||||
fs::create_dir_all(target.join("hooks"))?;
|
||||
fs::create_dir_all(&outside)?;
|
||||
fs::write(
|
||||
source.join("agents/researcher.md"),
|
||||
"---\nname: researcher\ndescription: Research\n---\nInstructions\n",
|
||||
)?;
|
||||
fs::write(
|
||||
source.join("settings.json"),
|
||||
r#"{"hooks":{"Stop":[{"hooks":[{"command":"echo done"}]}]}}"#,
|
||||
)?;
|
||||
fs::write(source.join("hooks/nested/escaped.sh"), "nested")?;
|
||||
fs::write(source.join("hooks/escaped.sh"), "dangling")?;
|
||||
fs::write(source.join("hooks/safe.sh"), "safe")?;
|
||||
std::os::unix::fs::symlink(
|
||||
outside.join("researcher.toml"),
|
||||
target.join("agents/researcher.toml"),
|
||||
)?;
|
||||
std::os::unix::fs::symlink(&outside, target.join("hooks/nested"))?;
|
||||
std::os::unix::fs::symlink(outside.join("escaped.sh"), target.join("hooks/escaped.sh"))?;
|
||||
|
||||
let service = service_for_paths(
|
||||
root.path().join(EXTERNAL_AGENT_DIR),
|
||||
root.path().join(".codex"),
|
||||
);
|
||||
let mut items = service
|
||||
.detect(ExternalAgentConfigDetectOptions {
|
||||
include_home: false,
|
||||
include_memory: false,
|
||||
cwds: Some(vec![repo.clone()]),
|
||||
})
|
||||
.await?;
|
||||
assert_eq!(
|
||||
items.iter().map(|item| item.item_type).collect::<Vec<_>>(),
|
||||
vec![ExternalAgentConfigMigrationItemType::Hooks]
|
||||
);
|
||||
items.push(ExternalAgentConfigMigrationItem {
|
||||
item_type: ExternalAgentConfigMigrationItemType::Subagents,
|
||||
description: String::new(),
|
||||
cwd: Some(repo),
|
||||
details: None,
|
||||
});
|
||||
let outcome = service.import(items).await;
|
||||
|
||||
assert_eq!(
|
||||
outcome
|
||||
.item_results
|
||||
.iter()
|
||||
.map(|result| (result.item_type, result.success_count, result.error_count))
|
||||
.collect::<Vec<_>>(),
|
||||
vec![
|
||||
(ExternalAgentConfigMigrationItemType::Hooks, 1, 0),
|
||||
(ExternalAgentConfigMigrationItemType::Subagents, 0, 0),
|
||||
]
|
||||
);
|
||||
assert_eq!(fs::read_to_string(target.join("hooks/safe.sh"))?, "safe");
|
||||
assert!(fs::read_dir(&outside)?.next().is_none());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn import_repo_agents_md_from_nested_cwd_rewrites_terms_and_skips_non_empty_targets() {
|
||||
let root = TempDir::new().expect("create tempdir");
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
use crate::RewriteProfile;
|
||||
use crate::invalid_data_error;
|
||||
use crate::scope::is_redirected_destination;
|
||||
use serde_yaml::Value as YamlValue;
|
||||
use std::collections::BTreeMap;
|
||||
use std::fs;
|
||||
@@ -46,7 +47,7 @@ pub fn missing_subagent_names(
|
||||
let Some(target) = subagent_target_file(&source_file, target_agents) else {
|
||||
continue;
|
||||
};
|
||||
if !target.exists() {
|
||||
if !target.exists() && !is_redirected_destination(&target)? {
|
||||
names.push(metadata.name);
|
||||
}
|
||||
}
|
||||
@@ -68,7 +69,7 @@ pub fn import_subagents_with_rewrite_profile(
|
||||
let Some(target) = subagent_target_file(&source_file, target_agents) else {
|
||||
continue;
|
||||
};
|
||||
if target.exists() {
|
||||
if target.exists() || is_redirected_destination(&target)? {
|
||||
continue;
|
||||
}
|
||||
let document = parse_document(&source_file)?;
|
||||
|
||||
Reference in New Issue
Block a user