From 788979771059c5c32dae93266715526ab968b90d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 5 Dec 2025 17:35:12 +0000 Subject: [PATCH 1/4] fix(acp): Fix mock agent path resolution for macOS CI tests The mock_acp_agent path resolution only went up one parent directory, which resolved to target/{arch}/{profile}/deps/mock_acp_agent instead of target/{arch}/{profile}/mock_acp_agent where the binary is actually placed. Changed to use .parent().and_then(|p| p.parent()) to correctly navigate from deps/ to the profile directory, matching the pattern used by codex_binary_path() in tui-pty-e2e. --- codex-rs/acp/src/registry.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/codex-rs/acp/src/registry.rs b/codex-rs/acp/src/registry.rs index 32ed71421..04b15dc32 100644 --- a/codex-rs/acp/src/registry.rs +++ b/codex-rs/acp/src/registry.rs @@ -65,13 +65,16 @@ pub fn get_agent_config(model_name: &str) -> Result { match normalized.as_str() { "mock-model" => { - // Use full path to mock_acp_agent binary from target directory - // This handles both debug and release builds + // Resolve path to mock_acp_agent binary relative to current executable. + // Test binaries are placed in target/{arch}/{profile}/deps/, while + // the mock_acp_agent binary is in target/{arch}/{profile}/. + // We need to go up two directories (deps -> profile dir) to find it. let exe_path = match std::env::current_exe() { Ok(p) => { let mock_path = p - .parent() - .map(|parent| parent.join("mock_acp_agent")) + .parent() // from test binary to deps/ + .and_then(|parent| parent.parent()) // from deps/ to profile dir + .map(|target_dir| target_dir.join("mock_acp_agent")) .unwrap_or_else(|| std::path::PathBuf::from("mock_acp_agent")); tracing::debug!("Mock ACP agent path resolved to: {}", mock_path.display()); mock_path From 52acf1d4f3c9e0ae152f05239c81360b0f53457b Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 5 Dec 2025 18:11:53 +0000 Subject: [PATCH 2/4] fix(acp): Handle both binary and test contexts for mock agent path The mock_acp_agent path resolution now detects the execution context: - When running as `codex` binary: parent dir is the profile dir - When running as test binary: parent dir is `deps/`, need two levels up This fixes cross-platform compatibility where Linux runs codex binary directly while macOS tests may run from the deps/ directory context. --- codex-rs/acp/src/registry.rs | 39 ++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/codex-rs/acp/src/registry.rs b/codex-rs/acp/src/registry.rs index 04b15dc32..a2b17873d 100644 --- a/codex-rs/acp/src/registry.rs +++ b/codex-rs/acp/src/registry.rs @@ -66,16 +66,39 @@ pub fn get_agent_config(model_name: &str) -> Result { match normalized.as_str() { "mock-model" => { // Resolve path to mock_acp_agent binary relative to current executable. - // Test binaries are placed in target/{arch}/{profile}/deps/, while - // the mock_acp_agent binary is in target/{arch}/{profile}/. - // We need to go up two directories (deps -> profile dir) to find it. + // + // There are two execution contexts: + // 1. Running as `codex` binary (E2E tests spawn codex): + // current_exe = target/{arch}/{profile}/codex + // -> one .parent() reaches the profile dir + // 2. Running as test binary (unit tests in deps/): + // current_exe = target/{arch}/{profile}/deps/test_binary + // -> two .parent() calls needed to reach profile dir + // + // We detect the context by checking if parent directory is named "deps". let exe_path = match std::env::current_exe() { Ok(p) => { - let mock_path = p - .parent() // from test binary to deps/ - .and_then(|parent| parent.parent()) // from deps/ to profile dir - .map(|target_dir| target_dir.join("mock_acp_agent")) - .unwrap_or_else(|| std::path::PathBuf::from("mock_acp_agent")); + let mock_path = if let Some(parent) = p.parent() { + // Check if we're in a "deps" directory (test binary context) + let in_deps_dir = parent + .file_name() + .map(|name| name == "deps") + .unwrap_or(false); + + let profile_dir = if in_deps_dir { + // Test binary: go up two levels (deps -> profile dir) + parent.parent() + } else { + // Codex binary: already one level up from profile dir + Some(parent) + }; + + profile_dir + .map(|dir| dir.join("mock_acp_agent")) + .unwrap_or_else(|| std::path::PathBuf::from("mock_acp_agent")) + } else { + std::path::PathBuf::from("mock_acp_agent") + }; tracing::debug!("Mock ACP agent path resolved to: {}", mock_path.display()); mock_path } From b45bb2f41c599d2e57c86c02e53755ab3b6debd0 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 5 Dec 2025 19:23:09 +0000 Subject: [PATCH 3/4] fix(ci): Build mock-acp-agent to known path for cross-platform tests The mock_acp_agent binary location varies across platforms due to different cargo target directory structures. This fix: 1. Adds a CI step to build mock-acp-agent with --artifact-dir to place the binary in a predictable location (target/mock-acp-out/) 2. Sets MOCK_ACP_AGENT_BIN environment variable pointing to the binary 3. Updates registry.rs to check MOCK_ACP_AGENT_BIN first, falling back to relative path resolution for local development This ensures consistent mock agent discovery on both Linux and macOS CI. --- .github/workflows/rust-ci.yml | 8 +++++ codex-rs/acp/src/registry.rs | 63 +++++++++++++++-------------------- 2 files changed, 35 insertions(+), 36 deletions(-) diff --git a/.github/workflows/rust-ci.yml b/.github/workflows/rust-ci.yml index 95f65f452..1195b0308 100644 --- a/.github/workflows/rust-ci.yml +++ b/.github/workflows/rust-ci.yml @@ -82,8 +82,16 @@ jobs: - name: cargo clippy run: cargo clippy --target ${{ matrix.target }} --all-features -- -D warnings + # Build mock-acp-agent to a known location for E2E tests + - name: Build mock-acp-agent + run: | + mkdir -p target/mock-acp-out + cargo +nightly build -p mock-acp-agent --profile ci-test --target ${{ matrix.target }} --artifact-dir target/mock-acp-out -Z unstable-options + - name: cargo test # continue-on-error: true # TODO: Fix pre-existing test failures in codex-app-server + env: + MOCK_ACP_AGENT_BIN: ${{ github.workspace }}/codex-rs/target/mock-acp-out/mock_acp_agent run: cargo test --profile ci-test --target ${{ matrix.target }} --all-features # Save cargo home cache diff --git a/codex-rs/acp/src/registry.rs b/codex-rs/acp/src/registry.rs index a2b17873d..4f8593b8e 100644 --- a/codex-rs/acp/src/registry.rs +++ b/codex-rs/acp/src/registry.rs @@ -65,50 +65,41 @@ pub fn get_agent_config(model_name: &str) -> Result { match normalized.as_str() { "mock-model" => { - // Resolve path to mock_acp_agent binary relative to current executable. + // Resolve path to mock_acp_agent binary. // - // There are two execution contexts: - // 1. Running as `codex` binary (E2E tests spawn codex): - // current_exe = target/{arch}/{profile}/codex - // -> one .parent() reaches the profile dir - // 2. Running as test binary (unit tests in deps/): - // current_exe = target/{arch}/{profile}/deps/test_binary - // -> two .parent() calls needed to reach profile dir - // - // We detect the context by checking if parent directory is named "deps". - let exe_path = match std::env::current_exe() { - Ok(p) => { - let mock_path = if let Some(parent) = p.parent() { + // Priority: + // 1. MOCK_ACP_AGENT_BIN environment variable (set by CI) + // 2. Relative to current executable (for local development) + let exe_path = if let Ok(env_path) = std::env::var("MOCK_ACP_AGENT_BIN") { + tracing::debug!( + "Mock ACP agent path from MOCK_ACP_AGENT_BIN: {}", + env_path + ); + std::path::PathBuf::from(env_path) + } else { + // Fall back to resolving relative to current executable. + // This handles both: + // - Running as `codex` binary: target/{profile}/codex -> target/{profile}/ + // - Running as test binary: target/{profile}/deps/test -> target/{profile}/ + let mock_path = std::env::current_exe() + .ok() + .and_then(|p| p.parent().map(|parent| { // Check if we're in a "deps" directory (test binary context) let in_deps_dir = parent .file_name() .map(|name| name == "deps") .unwrap_or(false); - let profile_dir = if in_deps_dir { - // Test binary: go up two levels (deps -> profile dir) - parent.parent() + if in_deps_dir { + parent.parent().map(|p| p.join("mock_acp_agent")) } else { - // Codex binary: already one level up from profile dir - Some(parent) - }; - - profile_dir - .map(|dir| dir.join("mock_acp_agent")) - .unwrap_or_else(|| std::path::PathBuf::from("mock_acp_agent")) - } else { - std::path::PathBuf::from("mock_acp_agent") - }; - tracing::debug!("Mock ACP agent path resolved to: {}", mock_path.display()); - mock_path - } - Err(e) => { - tracing::warn!( - "Failed to get current_exe for mock-model: {}, falling back to 'mock_acp_agent'", - e - ); - std::path::PathBuf::from("mock_acp_agent") - } + Some(parent.join("mock_acp_agent")) + } + })) + .flatten() + .unwrap_or_else(|| std::path::PathBuf::from("mock_acp_agent")); + tracing::debug!("Mock ACP agent path resolved to: {}", mock_path.display()); + mock_path }; Ok(AcpAgentConfig { From c7ab5d6db30f79d3624305f66d0c0314e68ce56e Mon Sep 17 00:00:00 2001 From: Clifford Ressel Date: Fri, 5 Dec 2025 16:26:40 -0500 Subject: [PATCH 4/4] Fix fmt --- codex-rs/acp/src/registry.rs | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/codex-rs/acp/src/registry.rs b/codex-rs/acp/src/registry.rs index 4f8593b8e..e197ec825 100644 --- a/codex-rs/acp/src/registry.rs +++ b/codex-rs/acp/src/registry.rs @@ -71,10 +71,7 @@ pub fn get_agent_config(model_name: &str) -> Result { // 1. MOCK_ACP_AGENT_BIN environment variable (set by CI) // 2. Relative to current executable (for local development) let exe_path = if let Ok(env_path) = std::env::var("MOCK_ACP_AGENT_BIN") { - tracing::debug!( - "Mock ACP agent path from MOCK_ACP_AGENT_BIN: {}", - env_path - ); + tracing::debug!("Mock ACP agent path from MOCK_ACP_AGENT_BIN: {}", env_path); std::path::PathBuf::from(env_path) } else { // Fall back to resolving relative to current executable. @@ -83,19 +80,21 @@ pub fn get_agent_config(model_name: &str) -> Result { // - Running as test binary: target/{profile}/deps/test -> target/{profile}/ let mock_path = std::env::current_exe() .ok() - .and_then(|p| p.parent().map(|parent| { - // Check if we're in a "deps" directory (test binary context) - let in_deps_dir = parent - .file_name() - .map(|name| name == "deps") - .unwrap_or(false); - - if in_deps_dir { - parent.parent().map(|p| p.join("mock_acp_agent")) - } else { - Some(parent.join("mock_acp_agent")) - } - })) + .and_then(|p| { + p.parent().map(|parent| { + // Check if we're in a "deps" directory (test binary context) + let in_deps_dir = parent + .file_name() + .map(|name| name == "deps") + .unwrap_or(false); + + if in_deps_dir { + parent.parent().map(|p| p.join("mock_acp_agent")) + } else { + Some(parent.join("mock_acp_agent")) + } + }) + }) .flatten() .unwrap_or_else(|| std::path::PathBuf::from("mock_acp_agent")); tracing::debug!("Mock ACP agent path resolved to: {}", mock_path.display());