From 2a30972fcb646f9d6bfed6de6606aa3f0f8c3dd2 Mon Sep 17 00:00:00 2001 From: jif Date: Tue, 18 Aug 2026 15:58:59 +0000 Subject: [PATCH] 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 --- .../tests/suite/v2/external_agent_config.rs | 83 +++++++++++++++++++ .../src/hooks_common.rs | 9 +- .../external-agent-migration/src/scope.rs | 39 ++++++++- .../src/service_tests/general/repo_import.rs | 71 ++++++++++++++++ .../external-agent-migration/src/subagents.rs | 5 +- 5 files changed, 202 insertions(+), 5 deletions(-) diff --git a/codex-rs/app-server/tests/suite/v2/external_agent_config.rs b/codex-rs/app-server/tests/suite/v2/external_agent_config.rs index 5c66e82fe8..de6db7d667 100644 --- a/codex-rs/app-server/tests/suite/v2/external_agent_config.rs +++ b/codex-rs/app-server/tests/suite/v2/external_agent_config.rs @@ -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<()> { diff --git a/codex-rs/external-agent-migration/src/hooks_common.rs b/codex-rs/external-agent-migration/src/hooks_common.rs index 5fdf288c90..527b4b6d37 100644 --- a/codex-rs/external-agent-migration/src/hooks_common.rs +++ b/codex-rs/external-agent-migration/src/hooks_common.rs @@ -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)?; } } diff --git a/codex-rs/external-agent-migration/src/scope.rs b/codex-rs/external-agent-migration/src/scope.rs index f4b6556a12..7a3824a1ba 100644 --- a/codex-rs/external-agent-migration/src/scope.rs +++ b/codex-rs/external-agent-migration/src/scope.rs @@ -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> { + 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 { + 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; diff --git a/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs b/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs index 8321b52e39..19ef149e42 100644 --- a/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs +++ b/codex-rs/external-agent-migration/src/service_tests/general/repo_import.rs @@ -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![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![ + (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"); diff --git a/codex-rs/external-agent-migration/src/subagents.rs b/codex-rs/external-agent-migration/src/subagents.rs index 4f27270408..949079936e 100644 --- a/codex-rs/external-agent-migration/src/subagents.rs +++ b/codex-rs/external-agent-migration/src/subagents.rs @@ -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)?;