mirror of
https://github.com/openai/codex.git
synced 2026-05-04 03:16:31 +00:00
test: stabilize app-server path assertions on Windows (#19604)
## Why Windows can represent the same canonical local path with either a normal drive path or a verbatim device path prefix. The failure pattern that motivated this PR was an assertion diff like `C:\...` versus `\\?\C:\...`: different spellings, same file. That became visible while validating the permissions stack above this PR. The stack increasingly routes paths through `AbsolutePathBuf`, which normalizes supported Windows device prefixes, while several existing tests still built expected values directly with `std::fs::canonicalize()` or compared `AbsolutePathBuf::as_path()` to a raw `PathBuf`. On Windows, that can make tests fail because the two sides choose different textual forms for an otherwise equivalent canonical path. This PR is intentionally split out as the bottom PR below #19606. The runtime permissions migration should not carry unrelated Windows test stabilization, and reviewers should be able to verify this as a test-only change before looking at the larger permissions changes. ## Failure Modes Covered - `conversation_summary` expected rollout paths were built from raw canonicalized `PathBuf`s, while app-server responses could carry `AbsolutePathBuf`-normalized paths. - `thread_resume` compared returned thread paths directly to previously stored or fixture paths, so a verbatim-prefix spelling could fail an otherwise correct resume. - `marketplace_add` compared plugin install roots through `as_path()` against raw canonicalized paths, reproducing the same `C:\...` versus `\\?\C:\...` mismatch in both app-server and core-plugin coverage. ## What Changed - In `app-server/tests/suite/conversation_summary.rs`, normalize both expected rollout paths and received `ConversationSummary.path` values through `AbsolutePathBuf` before comparing the full summary object. - In `app-server/tests/suite/v2/thread_resume.rs`, normalize both sides of thread path comparisons before asserting equality. This keeps the tests focused on whether resume returned the same existing path, not whether Windows used the same string spelling. - In `app-server/tests/suite/v2/marketplace_add.rs` and `core-plugins/src/marketplace_add.rs`, compare install roots as `AbsolutePathBuf` values instead of comparing an absolute-path wrapper to a raw canonicalized `PathBuf`. ## Behavior This PR does not change production app-server or marketplace behavior. It only changes tests to assert semantic path identity across Windows path spelling variants. It also leaves API response values untouched; the normalization happens inside assertions only. ## Verification Targeted local checks run while extracting this fix: - `cargo test -p codex-app-server get_conversation_summary_by_thread_id_reads_rollout` - `cargo test -p codex-app-server get_conversation_summary_by_relative_rollout_path_resolves_from_codex_home` - `cargo test -p codex-app-server thread_resume_prefers_path_over_thread_id` Windows-specific confidence comes from the Bazel Windows CI job for this PR, since the failure is platform-specific. ## Docs No docs update is needed because this is test-only infrastructure stabilization. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19604). * #19395 * #19394 * #19393 * #19392 * #19606 * __->__ #19604
This commit is contained in:
@@ -11,7 +11,9 @@ use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::path::Path;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::timeout;
|
||||
@@ -40,6 +42,15 @@ fn expected_summary(conversation_id: ThreadId, path: PathBuf) -> ConversationSum
|
||||
}
|
||||
}
|
||||
|
||||
fn normalized_canonical_path(path: impl AsRef<Path>) -> Result<PathBuf> {
|
||||
Ok(AbsolutePathBuf::from_absolute_path(path.as_ref().canonicalize()?)?.into_path_buf())
|
||||
}
|
||||
|
||||
fn normalized_summary_path(mut summary: ConversationSummary) -> Result<ConversationSummary> {
|
||||
summary.path = normalized_canonical_path(&summary.path)?;
|
||||
Ok(summary)
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn get_conversation_summary_by_thread_id_reads_rollout() -> Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
@@ -54,7 +65,7 @@ async fn get_conversation_summary_by_thread_id_reads_rollout() -> Result<()> {
|
||||
let thread_id = ThreadId::from_string(&conversation_id)?;
|
||||
let expected = expected_summary(
|
||||
thread_id,
|
||||
std::fs::canonicalize(rollout_path(
|
||||
normalized_canonical_path(rollout_path(
|
||||
codex_home.path(),
|
||||
FILENAME_TS,
|
||||
&conversation_id,
|
||||
@@ -76,7 +87,7 @@ async fn get_conversation_summary_by_thread_id_reads_rollout() -> Result<()> {
|
||||
.await??;
|
||||
let received: GetConversationSummaryResponse = to_response(response)?;
|
||||
|
||||
assert_eq!(received.summary, expected);
|
||||
assert_eq!(normalized_summary_path(received.summary)?, expected);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -126,7 +137,7 @@ async fn get_conversation_summary_by_relative_rollout_path_resolves_from_codex_h
|
||||
let thread_id = ThreadId::from_string(&conversation_id)?;
|
||||
let rollout_path = rollout_path(codex_home.path(), FILENAME_TS, &conversation_id);
|
||||
let relative_path = rollout_path.strip_prefix(codex_home.path())?.to_path_buf();
|
||||
let expected = expected_summary(thread_id, std::fs::canonicalize(rollout_path)?);
|
||||
let expected = expected_summary(thread_id, normalized_canonical_path(rollout_path)?);
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
@@ -143,6 +154,6 @@ async fn get_conversation_summary_by_relative_rollout_path_resolves_from_codex_h
|
||||
.await??;
|
||||
let received: GetConversationSummaryResponse = to_response(response)?;
|
||||
|
||||
assert_eq!(received.summary, expected);
|
||||
assert_eq!(normalized_summary_path(received.summary)?, expected);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -5,6 +5,7 @@ use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::MarketplaceAddParams;
|
||||
use codex_app_server_protocol::MarketplaceAddResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use tempfile::TempDir;
|
||||
use tokio::time::Duration;
|
||||
@@ -48,10 +49,10 @@ async fn marketplace_add_local_directory_source() -> Result<()> {
|
||||
installed_root,
|
||||
already_added,
|
||||
} = to_response(response)?;
|
||||
let expected_root = source.canonicalize()?;
|
||||
let expected_root = AbsolutePathBuf::from_absolute_path(source.canonicalize()?)?;
|
||||
|
||||
assert_eq!(marketplace_name, "debug");
|
||||
assert_eq!(installed_root.as_path(), expected_root.as_path());
|
||||
assert_eq!(installed_root, expected_root);
|
||||
assert!(!already_added);
|
||||
assert_eq!(
|
||||
std::fs::read_to_string(installed_root.as_path().join("plugins/sample/marker.txt"))?,
|
||||
|
||||
@@ -65,6 +65,7 @@ use codex_protocol::protocol::TurnStartedEvent;
|
||||
use codex_protocol::user_input::ByteRange;
|
||||
use codex_protocol::user_input::TextElement;
|
||||
use codex_state::StateRuntime;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -94,6 +95,10 @@ const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs
|
||||
const INTERNAL_ERROR_CODE: i64 = -32603;
|
||||
const CODEX_5_2_INSTRUCTIONS_TEMPLATE_DEFAULT: &str = "You are Codex, a coding agent based on GPT-5. You and the user share the same workspace and collaborate to achieve the user's goals.";
|
||||
|
||||
fn normalized_existing_path(path: impl AsRef<Path>) -> Result<PathBuf> {
|
||||
Ok(AbsolutePathBuf::from_absolute_path(path.as_ref().canonicalize()?)?.into_path_buf())
|
||||
}
|
||||
|
||||
async fn wait_for_responses_request_count(
|
||||
server: &wiremock::MockServer,
|
||||
expected_count: usize,
|
||||
@@ -2537,7 +2542,12 @@ async fn thread_resume_prefers_path_over_thread_id() -> Result<()> {
|
||||
thread: resumed, ..
|
||||
} = to_response::<ThreadResumeResponse>(resume_resp)?;
|
||||
assert_eq!(resumed.id, thread.id);
|
||||
assert_eq!(resumed.path, thread.path);
|
||||
let resumed_path = resumed.path.as_ref().expect("resumed thread path");
|
||||
let original_path = thread.path.as_ref().expect("original thread path");
|
||||
assert_eq!(
|
||||
normalized_existing_path(resumed_path)?,
|
||||
normalized_existing_path(original_path)?
|
||||
);
|
||||
assert_eq!(resumed.status, ThreadStatus::Idle);
|
||||
|
||||
Ok(())
|
||||
@@ -2577,9 +2587,12 @@ async fn thread_resume_can_load_source_by_external_path() -> Result<()> {
|
||||
let ThreadResumeResponse {
|
||||
thread: resumed, ..
|
||||
} = to_response::<ThreadResumeResponse>(resume_resp)?;
|
||||
let expected_thread_path = std::fs::canonicalize(&thread_path)?;
|
||||
assert_eq!(resumed.id, thread_id);
|
||||
assert_eq!(resumed.path, Some(expected_thread_path));
|
||||
let resumed_path = resumed.path.as_ref().expect("resumed thread path");
|
||||
assert_eq!(
|
||||
normalized_existing_path(resumed_path)?,
|
||||
normalized_existing_path(&thread_path)?
|
||||
);
|
||||
assert_eq!(resumed.preview, "external path history");
|
||||
assert_eq!(resumed.status, ThreadStatus::Idle);
|
||||
|
||||
|
||||
Reference in New Issue
Block a user