diff --git a/codex-rs/apply-patch/src/lib.rs b/codex-rs/apply-patch/src/lib.rs index 81ad1f0781..81536f4a9d 100644 --- a/codex-rs/apply-patch/src/lib.rs +++ b/codex-rs/apply-patch/src/lib.rs @@ -223,7 +223,7 @@ impl Default for AppliedPatchDelta { /// A committed file change, preserved in the order it was applied. #[derive(Clone, Debug, PartialEq)] pub struct AppliedPatchChange { - pub path: PathBuf, + pub path: PathUri, pub change: AppliedPatchFileChange, } @@ -237,7 +237,7 @@ pub enum AppliedPatchFileChange { content: String, }, Update { - move_path: Option, + move_path: Option, old_content: String, overwritten_move_content: Option, new_content: String, @@ -387,7 +387,6 @@ async fn apply_hunks_to_files( }; } - // TODO(anp): Carry PathUri through committed patch deltas and the turn diff tracker. for hunk in hunks { let affected_path = hunk.path().to_path_buf(); let path_uri = hunk.resolve_path(cwd)?; @@ -406,7 +405,7 @@ async fn apply_hunks_to_files( .await ); delta.changes.push(AppliedPatchChange { - path: path_uri.to_path_buf(), + path: path_uri, change: AppliedPatchFileChange::Add { content: contents.clone(), overwritten_content, @@ -456,7 +455,7 @@ async fn apply_hunks_to_files( } if let Some(content) = deleted_content { delta.changes.push(AppliedPatchChange { - path: path_uri.to_path_buf(), + path: path_uri, change: AppliedPatchFileChange::Delete { content }, }); } @@ -486,7 +485,7 @@ async fn apply_hunks_to_files( ); let dest_write_change_index = delta.changes.len(); delta.changes.push(AppliedPatchChange { - path: dest_uri.to_path_buf(), + path: dest_uri.clone(), change: AppliedPatchFileChange::Add { content: new_contents.clone(), overwritten_content: overwritten_move_content.clone(), @@ -527,9 +526,9 @@ async fn apply_hunks_to_files( return Err(error); } delta.changes[dest_write_change_index] = AppliedPatchChange { - path: path_uri.to_path_buf(), + path: path_uri, change: AppliedPatchFileChange::Update { - move_path: Some(dest_uri.to_path_buf()), + move_path: Some(dest_uri), old_content: original_contents, overwritten_move_content, new_content: new_contents, @@ -546,7 +545,7 @@ async fn apply_hunks_to_files( )) ); delta.changes.push(AppliedPatchChange { - path: path_uri.to_path_buf(), + path: path_uri, change: AppliedPatchFileChange::Update { move_path: None, old_content: original_contents, @@ -1156,7 +1155,7 @@ mod tests { failure.delta(), &AppliedPatchDelta::new( vec![AppliedPatchChange { - path: dest.clone(), + path: PathUri::from_host_native_path(&dest).expect("absolute destination path"), change: AppliedPatchFileChange::Add { content: "line2\n".to_string(), overwritten_content: None, diff --git a/codex-rs/core/src/git_info_tests.rs b/codex-rs/core/src/git_info_tests.rs index d056a6e626..bebcd0f25a 100644 --- a/codex-rs/core/src/git_info_tests.rs +++ b/codex-rs/core/src/git_info_tests.rs @@ -12,7 +12,6 @@ use codex_exec_server::RemoveOptions; use codex_git_utils::GitInfo; use codex_git_utils::GitSha; use codex_git_utils::collect_git_info; -use codex_git_utils::get_git_repo_root_with_fs; use codex_git_utils::get_has_changes; use codex_git_utils::git_diff_to_remote; use codex_git_utils::recent_commits; @@ -602,21 +601,7 @@ async fn resolve_root_git_project_for_trust_returns_none_outside_repo() { } #[tokio::test] -async fn get_git_repo_root_with_fs_detects_gitdir_pointer() { - let tmp = TempDir::new().expect("tempdir"); - let proj = tmp.path().join("proj"); - let nested = proj.join("nested"); - std::fs::create_dir_all(&nested).unwrap(); - std::fs::write(proj.join(".git"), "gitdir: /tmp/fake-worktree\n").unwrap(); - - assert_eq!( - get_git_repo_root_with_fs(LOCAL_FS.as_ref(), &nested.abs()).await, - Some(proj.abs()) - ); -} - -#[tokio::test] -async fn get_git_repo_root_with_fs_starts_at_parent_for_file() { +async fn resolve_root_git_project_for_trust_starts_at_parent_for_file() { let tmp = TempDir::new().expect("tempdir"); let proj = tmp.path().join("proj"); let nested = proj.join("nested"); @@ -626,13 +611,13 @@ async fn get_git_repo_root_with_fs_starts_at_parent_for_file() { std::fs::write(&file, "contents").unwrap(); assert_eq!( - get_git_repo_root_with_fs(LOCAL_FS.as_ref(), &file.abs()).await, + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &file.abs()).await, Some(proj.abs()) ); } #[tokio::test] -async fn get_git_repo_root_with_fs_ignores_metadata_errors() { +async fn resolve_root_git_project_for_trust_ignores_metadata_errors() { let tmp = TempDir::new().expect("tempdir"); let proj = tmp.path().join("proj"); let nested = proj.join("nested"); @@ -643,14 +628,14 @@ async fn get_git_repo_root_with_fs_ignores_metadata_errors() { }; assert_eq!( - get_git_repo_root_with_fs(&fs, &nested.abs()).await, + resolve_root_git_project_for_trust(&fs, &nested.abs()).await, Some(proj.abs()) ); } #[cfg(windows)] #[tokio::test] -async fn get_git_repo_root_with_fs_supports_windows_namespace_paths() { +async fn resolve_root_git_project_for_trust_supports_windows_namespace_paths() { let tmp = TempDir::new().expect("tempdir"); let repo = tmp.path().join("repo"); std::fs::create_dir_all(repo.join(".git")).unwrap(); @@ -660,7 +645,7 @@ async fn get_git_repo_root_with_fs_supports_windows_namespace_paths() { let namespace_nested = namespace_repo.join("nested"); assert_eq!( - get_git_repo_root_with_fs(LOCAL_FS.as_ref(), &namespace_nested.abs()).await, + resolve_root_git_project_for_trust(LOCAL_FS.as_ref(), &namespace_nested.abs()).await, Some(namespace_repo.abs()) ); } diff --git a/codex-rs/core/src/session/turn.rs b/codex-rs/core/src/session/turn.rs index ac3686aef5..a696c13322 100644 --- a/codex-rs/core/src/session/turn.rs +++ b/codex-rs/core/src/session/turn.rs @@ -1,6 +1,5 @@ use std::collections::HashMap; use std::collections::HashSet; -use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::Ordering; @@ -79,7 +78,8 @@ use codex_core_skills::injection::InjectedHostSkillPrompts; use codex_extension_api::TurnInputContext; use codex_extension_api::TurnInputEnvironment; use codex_features::Feature; -use codex_git_utils::get_git_repo_root_with_fs; +use codex_file_system::FindUpErrorPolicy; +use codex_file_system::find_nearest_ancestor_with_markers; use codex_protocol::ResponseItemId; use codex_protocol::config_types::AutoCompactTokenLimitScope; use codex_protocol::config_types::ModeKind; @@ -109,6 +109,7 @@ use codex_protocol::protocol::WarningEvent; use codex_protocol::user_input::UserInput; use codex_tools::ToolName; use codex_tools::filter_request_plugin_install_discoverable_tools_for_client; +use codex_utils_path_uri::PathUri; use codex_utils_stream_parser::AssistantTextChunk; use codex_utils_stream_parser::AssistantTextStreamParser; use codex_utils_stream_parser::ProposedPlanSegment; @@ -171,7 +172,7 @@ pub(crate) async fn run_turn( // Keep the exact model-visible state used by this turn and its inline compactions. let (mut world_state, display_roots) = tokio::join!( sess.record_context_updates_and_set_reference_context_item(first_step_context.as_ref()), - turn_diff_display_roots(turn_context.as_ref()), + turn_diff_display_roots(first_step_context.as_ref()), ); let Some((injection_items, explicitly_enabled_connectors)) = build_skills_and_plugins( @@ -468,19 +469,23 @@ pub(crate) async fn run_turn( } #[instrument(level = "trace", skip_all)] -async fn turn_diff_display_roots(turn_context: &TurnContext) -> Vec<(String, PathBuf)> { +async fn turn_diff_display_roots(step_context: &StepContext) -> Vec<(String, PathUri)> { let mut display_roots = Vec::new(); - for turn_environment in turn_context.environments.turn_environments() { - // TODO(anp): Migrate git-root discovery and diff display roots to PathUri so foreign - // environment roots can participate without host-native conversion. - let Ok(cwd) = turn_environment.cwd().to_abs_path() else { - continue; - }; - let root = - get_git_repo_root_with_fs(turn_environment.environment.get_filesystem().as_ref(), &cwd) - .await - .unwrap_or(cwd) - .into_path_buf(); + for turn_environment in step_context.environments.turn_environments() { + let cwd = turn_environment.cwd(); + // A turn cwd is expected to be a directory. If it is a file, the failed `/.git` probe + // is ignored and ancestor search continues from its parent. + let root = find_nearest_ancestor_with_markers( + turn_environment.environment.get_filesystem().as_ref(), + cwd, + vec![".git".to_string()], + FindUpErrorPolicy::Ignore, + /*sandbox*/ None, + ) + .await + .ok() + .flatten() + .unwrap_or_else(|| cwd.clone()); display_roots.push((turn_environment.environment_id.clone(), root)); } display_roots diff --git a/codex-rs/core/src/turn_diff_tracker.rs b/codex-rs/core/src/turn_diff_tracker.rs index ed8e9684ad..06a2cd022e 100644 --- a/codex-rs/core/src/turn_diff_tracker.rs +++ b/codex-rs/core/src/turn_diff_tracker.rs @@ -1,7 +1,5 @@ use std::collections::HashMap; use std::collections::HashSet; -use std::path::Path; -use std::path::PathBuf; use std::time::Duration; use sha1::digest::Output; @@ -9,6 +7,7 @@ use sha1::digest::Output; use codex_apply_patch::AppliedPatchChange; use codex_apply_patch::AppliedPatchDelta; use codex_apply_patch::AppliedPatchFileChange; +use codex_utils_path_uri::PathUri; const ZERO_OID: &str = "0000000000000000000000000000000000000000"; const DEV_NULL: &str = "/dev/null"; @@ -25,14 +24,14 @@ struct TrackedContent { #[derive(Clone, Debug, Eq, Hash, PartialEq)] struct TrackedPath { environment_id: String, - path: PathBuf, + path: PathUri, } impl TrackedPath { - fn new(environment_id: &str, path: &Path) -> Self { + fn new(environment_id: &str, path: &PathUri) -> Self { Self { environment_id: environment_id.to_string(), - path: path.to_path_buf(), + path: path.clone(), } } } @@ -49,7 +48,7 @@ struct DiffCacheKey { /// mutations, without rereading the workspace filesystem. pub struct TurnDiffTracker { valid: bool, - display_roots_by_environment: HashMap, + display_roots_by_environment: HashMap, baseline_by_path: HashMap, current_by_path: HashMap, origin_by_current_path: HashMap, @@ -83,7 +82,7 @@ impl TurnDiffTracker { } pub fn with_environment_display_roots( - display_roots: impl IntoIterator, + display_roots: impl IntoIterator, ) -> Self { let mut tracker = Self::new(); tracker.display_roots_by_environment = display_roots.into_iter().collect(); @@ -183,7 +182,7 @@ impl TurnDiffTracker { } fn apply_change(&mut self, environment_id: &str, change: &AppliedPatchChange) { - let source_path = TrackedPath::new(environment_id, change.path.as_path()); + let source_path = TrackedPath::new(environment_id, &change.path); match &change.change { AppliedPatchFileChange::Add { content, @@ -197,7 +196,7 @@ impl TurnDiffTracker { new_content, } => { let move_path = move_path - .as_deref() + .as_ref() .map(|path| TrackedPath::new(environment_id, path)); self.apply_update( source_path, @@ -321,8 +320,9 @@ impl TurnDiffTracker { self.rendered_diff_count .set(self.rendered_diff_count.get() + 1); - let left_display = self.display_path(left_path); - let right_display = self.display_path(right_path); + // Git diff paths always use `/`, even when the displayed target path is Windows-native. + let left_display = self.display_path(left_path).replace('\\', "/"); + let right_display = self.display_path(right_path).replace('\\', "/"); let left_oid = left_content.map_or_else( || ZERO_OID.to_string(), |content| git_blob_oid(content.as_bytes()), @@ -374,9 +374,8 @@ impl TurnDiffTracker { let display = self .display_roots_by_environment .get(&path.environment_id) - .and_then(|root| path.path.strip_prefix(root).ok()) - .unwrap_or(path.path.as_path()); - let display = display.display().to_string().replace('\\', "/"); + .and_then(|root| path.path.relative_path_from(root)) + .unwrap_or_else(|| path.path.inferred_native_path_string()); if self.display_roots_by_environment.len() > 1 && !path.environment_id.is_empty() { format!("{}/{display}", path.environment_id) } else { diff --git a/codex-rs/core/src/turn_diff_tracker_tests.rs b/codex-rs/core/src/turn_diff_tracker_tests.rs index 5f984a2c4f..27af3ff32b 100644 --- a/codex-rs/core/src/turn_diff_tracker_tests.rs +++ b/codex-rs/core/src/turn_diff_tracker_tests.rs @@ -47,7 +47,10 @@ async fn apply_verified_patch(root: &Path, patch: &str) -> AppliedPatchDelta { } fn tracker_with_root(root: &Path) -> TurnDiffTracker { - TurnDiffTracker::with_environment_display_roots([("".to_string(), root.to_path_buf())]) + TurnDiffTracker::with_environment_display_roots([( + "".to_string(), + PathUri::from_host_native_path(root).expect("absolute display root"), + )]) } #[tokio::test] @@ -111,8 +114,14 @@ async fn tracks_same_absolute_path_across_multiple_environments() { .await; let mut tracker = TurnDiffTracker::with_environment_display_roots([ - ("local".to_string(), dir.path().to_path_buf()), - ("remote".to_string(), dir.path().to_path_buf()), + ( + "local".to_string(), + PathUri::from_host_native_path(dir.path()).expect("absolute local display root"), + ), + ( + "remote".to_string(), + PathUri::from_host_native_path(dir.path()).expect("absolute remote display root"), + ), ]); tracker.track_delta("remote", &add); tracker.track_delta("local", &add); @@ -138,6 +147,21 @@ index {ZERO_OID}..{right_oid} assert_eq!(tracker.get_unified_diff(), Some(expected)); } +#[test] +fn displays_foreign_paths_relative_to_their_environment_root() { + let tracker = TurnDiffTracker::with_environment_display_roots([( + "windows".to_string(), + PathUri::parse("file:///C:/workspace/project").expect("valid Windows display root"), + )]); + let path = TrackedPath::new( + "windows", + &PathUri::parse("file:///C:/workspace/project/src/main.rs") + .expect("valid Windows file path"), + ); + + assert_eq!(tracker.display_path(&path), r"src\main.rs"); +} + #[tokio::test] async fn accumulates_delete() { let dir = tempdir().expect("tempdir"); @@ -463,7 +487,8 @@ fn large_rewrite_returns_promptly_and_preserves_exact_content() { .success() ); let tracker = tracker_with_root(dir.path()); - let tracked_path = TrackedPath::new("", &path); + let path_uri = PathUri::from_host_native_path(&path).expect("absolute tracked path"); + let tracked_path = TrackedPath::new("", &path_uri); let started = Instant::now(); let diff = tracker diff --git a/codex-rs/core/tests/suite/apply_patch_cli.rs b/codex-rs/core/tests/suite/apply_patch_cli.rs index 7fbe55d8c0..0c4b35f1f0 100644 --- a/codex-rs/core/tests/suite/apply_patch_cli.rs +++ b/codex-rs/core/tests/suite/apply_patch_cli.rs @@ -37,6 +37,7 @@ use codex_sandboxing::landlock::CODEX_LINUX_SANDBOX_ARG0; use codex_utils_absolute_path::AbsolutePathBuf; use codex_utils_path_uri::PathUri; use core_test_support::PathBufExt; +use core_test_support::TestTargetOs; use core_test_support::assert_regex_match; use core_test_support::responses::ev_assistant_message; use core_test_support::responses::ev_completed; @@ -56,6 +57,7 @@ use core_test_support::test_codex::TestCodexHarness; use core_test_support::test_codex::local; use core_test_support::test_codex::test_codex; use core_test_support::test_codex::turn_permission_fields; +use core_test_support::test_target_os; use core_test_support::wait_for_event; use core_test_support::wait_for_event_with_timeout; use serde_json::json; @@ -1582,6 +1584,63 @@ async fn apply_patch_emits_turn_diff_event_with_unified_diff() -> Result<()> { Ok(()) } +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn apply_patch_turn_diff_emits_portable_paths_for_remote_cwd() -> Result<()> { + skip_if_no_network!(Ok(())); + skip_if_no_remote_env!(Ok(())); + + let harness = apply_patch_harness().await?; + let test = harness.test(); + let codex = test.codex.clone(); + + let call_id = "apply-foreign-windows-diff"; + let file = "nested/foreign.txt"; + let patch = format!("*** Begin Patch\n*** Add File: {file}\n+hello\n*** End Patch"); + mount_apply_patch(&harness, call_id, &patch, "ok").await; + + submit_without_wait(&harness, "emit diff for a foreign Windows cwd").await?; + + let mut last_diff = None; + wait_for_event(&codex, |event| match event { + EventMsg::TurnDiff(ev) => { + last_diff = Some(ev.unified_diff.clone()); + false + } + EventMsg::TurnComplete(_) => true, + _ => false, + }) + .await; + + let cwd = &test.executor_environment().selection().cwd; + let file_uri = cwd.join(file)?; + let expected_relative_path = match test_target_os() { + TestTargetOs::Linux | TestTargetOs::MacOs => "nested/foreign.txt", + TestTargetOs::Windows => r"nested\foreign.txt", + }; + assert_eq!( + file_uri.relative_path_from(cwd).as_deref(), + Some(expected_relative_path) + ); + assert_eq!( + test.fs() + .read_file_text(&file_uri, /*sandbox*/ None) + .await?, + "hello\n" + ); + assert_eq!( + last_diff.expect("expected TurnDiff event"), + r#"diff --git a/nested/foreign.txt b/nested/foreign.txt +new file mode 100644 +index 0000000000000000000000000000000000000000..ce013625030ba8dba906f756967f9e9ca394464a +--- /dev/null ++++ b/nested/foreign.txt +@@ -0,0 +1 @@ ++hello +"# + ); + Ok(()) +} + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn apply_patch_turn_diff_tracks_local_and_remote_environment_paths() -> Result<()> { // TODO(anp): Remove after shared-cwd helpers use target-native paths. @@ -1789,8 +1848,8 @@ async fn apply_patch_aggregates_diff_across_multiple_tool_calls() -> Result<()> .await; let diff = last_diff.expect("expected TurnDiff after two patches"); - assert!(diff.contains("agg/a.txt"), "diff missing a.txt"); - assert!(diff.contains("agg/b.txt"), "diff missing b.txt"); + assert!(diff.contains("agg/a.txt"), "diff missing agg/a.txt: {diff}"); + assert!(diff.contains("agg/b.txt"), "diff missing agg/b.txt: {diff}"); // Final content reflects v2 for a.txt assert!(diff.contains("+v2\n") || diff.contains("v2\n")); Ok(()) diff --git a/codex-rs/git-utils/src/info.rs b/codex-rs/git-utils/src/info.rs index 6ef3f64dad..ff013525d4 100644 --- a/codex-rs/git-utils/src/info.rs +++ b/codex-rs/git-utils/src/info.rs @@ -42,30 +42,6 @@ pub fn get_git_repo_root(base_dir: &Path) -> Option { find_ancestor_git_entry(base).map(|(repo_root, _)| repo_root) } -/// Return the repository root for `cwd` using the provided filesystem. -/// -/// This mirrors [`get_git_repo_root`] for local paths, but works when `cwd` -/// only exists inside a selected remote environment. -pub async fn get_git_repo_root_with_fs( - fs: &dyn ExecutorFileSystem, - cwd: &AbsolutePathBuf, -) -> Option { - let cwd_uri = PathUri::from_abs_path(cwd); - let base = match fs.get_metadata(&cwd_uri, /*sandbox*/ None).await { - Ok(metadata) if metadata.is_directory => cwd.clone(), - _ => cwd.parent()?, - }; - find_nearest_native_ancestor_with_markers( - fs, - &base, - vec![".git".to_string()], - FindUpErrorPolicy::Ignore, - /*sandbox*/ None, - ) - .await - .ok()? -} - /// Timeout for git commands to prevent freezing on large repositories const GIT_COMMAND_TIMEOUT: TokioDuration = TokioDuration::from_secs(5); const DISABLED_HOOKS_PATH: &str = if cfg!(windows) { "NUL" } else { "/dev/null" }; @@ -817,7 +793,20 @@ pub async fn resolve_root_git_project_for_trust( fs: &dyn ExecutorFileSystem, cwd: &AbsolutePathBuf, ) -> Option { - let repo_root = get_git_repo_root_with_fs(fs, cwd).await?; + let cwd_uri = PathUri::from_abs_path(cwd); + let base = match fs.get_metadata(&cwd_uri, /*sandbox*/ None).await { + Ok(metadata) if metadata.is_directory => cwd.clone(), + _ => cwd.parent()?, + }; + let repo_root = find_nearest_native_ancestor_with_markers( + fs, + &base, + vec![".git".to_string()], + FindUpErrorPolicy::Ignore, + /*sandbox*/ None, + ) + .await + .ok()??; let dot_git = repo_root.join(".git"); let dot_git_uri = PathUri::from_abs_path(&dot_git); if fs diff --git a/codex-rs/git-utils/src/lib.rs b/codex-rs/git-utils/src/lib.rs index e04e604410..c91688470f 100644 --- a/codex-rs/git-utils/src/lib.rs +++ b/codex-rs/git-utils/src/lib.rs @@ -35,7 +35,6 @@ pub use info::default_branch_name; pub use info::get_git_remote_urls; pub use info::get_git_remote_urls_assume_git_repo; pub use info::get_git_repo_root; -pub use info::get_git_repo_root_with_fs; pub use info::get_has_changes; pub use info::get_head_commit_hash; pub use info::git_diff_to_remote; diff --git a/codex-rs/utils/path-uri/src/api_path_string_tests.rs b/codex-rs/utils/path-uri/src/api_path_string_tests.rs index c6ee1ea6ae..8fe6a7c085 100644 --- a/codex-rs/utils/path-uri/src/api_path_string_tests.rs +++ b/codex-rs/utils/path-uri/src/api_path_string_tests.rs @@ -135,7 +135,7 @@ const RENDER_CASES: &[RenderCase] = &[ RenderCase::round_trips( "file:///d:/snowman/%E2%98%83", PathConvention::Windows, - r"d:\snowman\☃", + r"D:\snowman\☃", ), RenderCase::round_trips("file:///C:/tmp/", PathConvention::Windows, "C:\\tmp\\"), RenderCase::round_trips( diff --git a/codex-rs/utils/path-uri/src/lib.rs b/codex-rs/utils/path-uri/src/lib.rs index ccc9cfd79b..80d1798a42 100644 --- a/codex-rs/utils/path-uri/src/lib.rs +++ b/codex-rs/utils/path-uri/src/lib.rs @@ -38,8 +38,9 @@ const BAD_PATH_URI_PREFIX: &str = "file:///%00/bad/path/"; /// created by [`Self::from_abs_path`] are opaque to these lexical operations. /// /// `file:` paths retain their URI spelling so they can be parsed independently -/// of the current host. A local POSIX `file:` URI can also retain -/// percent-encoded non-UTF-8 bytes for lossless native round trips. +/// of the current host, except that Windows drive letters are canonicalized to +/// uppercase. A local POSIX `file:` URI can also retain percent-encoded non-UTF-8 +/// bytes for lossless native round trips. /// /// Like [VS Code resources], path operations use `/` URI separators on every /// host. Lexical path operations preserve a URL authority without interpreting @@ -281,6 +282,43 @@ impl PathUri { path_segments.starts_with(&base_segments) } + /// Returns the decoded relative path from `base` to this URI. + /// + /// The result uses the separators of the inferred path convention, + /// independently of the current host. Both URIs must use the same inferred + /// path convention and authority, and this URI must be lexically equal to + /// or below `base`. Percent-encoded native path separators fail closed. + /// Opaque fallback URIs created by [`Self::from_abs_path`] are only relative + /// to themselves. + pub fn relative_path_from(&self, base: &Self) -> Option { + if self == base { + return Some(String::new()); + } + if decode_bad_path_uri(&self.0).is_some() + || decode_bad_path_uri(&base.0).is_some() + || self.0.host_str() != base.0.host_str() + || self.infer_path_convention() != base.infer_path_convention() + { + return None; + } + + let convention = self.infer_path_convention()?; + let path_segments = containment_path_segments(&self.0, convention)?; + let base_segments = containment_path_segments(&base.0, convention)?; + let relative_segments = path_segments.strip_prefix(base_segments.as_slice())?; + let separator = match convention { + PathConvention::Posix => "/", + PathConvention::Windows => "\\", + }; + Some( + relative_segments + .iter() + .map(|segment| decode_uri_path(segment)) + .collect::>() + .join(separator), + ) + } + /// Lexically resolves native absolute or relative path text against this URI. /// /// Path text is interpreted using the POSIX or Windows convention inferred @@ -453,6 +491,7 @@ impl TryFrom for PathUri { } validate_file_url(&url)?; let url = without_localhost_authority(url); + let url = with_normalized_windows_drive_letter(url); Ok(Self(url)) } } @@ -524,6 +563,32 @@ fn without_localhost_authority(mut url: Url) -> Url { url } +fn with_normalized_windows_drive_letter(mut url: Url) -> Url { + if url.host_str().is_some() { + return url; + } + + let path = url.path(); + let Some(drive_start) = path.bytes().position(|byte| byte != b'/') else { + return url; + }; + let Some(drive) = path[drive_start..].split('/').next() else { + return url; + }; + if !is_windows_drive_uri_segment(drive) || drive.as_bytes()[0].is_ascii_uppercase() { + return url; + } + + let drive_letter = char::from(drive.as_bytes()[0]).to_ascii_uppercase(); + let normalized_path = format!( + "{}{drive_letter}{}", + &path[..drive_start], + &path[drive_start + 1..] + ); + url.set_path(&normalized_path); + url +} + /// Percent-decodes a URI path when it is valid UTF-8. /// /// `file:` URLs may contain encoded non-UTF-8 bytes. In that case the encoded diff --git a/codex-rs/utils/path-uri/src/tests.rs b/codex-rs/utils/path-uri/src/tests.rs index 68113c8f60..548d740037 100644 --- a/codex-rs/utils/path-uri/src/tests.rs +++ b/codex-rs/utils/path-uri/src/tests.rs @@ -66,6 +66,20 @@ fn file_uri_parses_a_windows_path_on_any_host() { ); } +#[test] +fn file_uri_normalizes_windows_drive_letter_case() { + let lowercase = PathUri::parse("file:///c:/Users/Alice%20Smith/src/main.rs") + .expect("Windows file URI should parse"); + let uppercase = PathUri::parse("file:///C:/Users/Alice%20Smith/src/main.rs") + .expect("Windows file URI should parse"); + + assert_eq!(lowercase, uppercase); + assert_eq!( + lowercase.to_string(), + "file:///C:/Users/Alice%20Smith/src/main.rs" + ); +} + #[test] fn infers_path_conventions_from_uri_shape() { for (uri, expected) in [ @@ -158,6 +172,75 @@ fn inferred_native_path_string_uses_the_inferred_convention() { } } +#[test] +fn relative_path_from_is_host_independent() { + // `file://abc/...` has an authority and is inferred as Windows UNC, while + // `file:///abc/...` is hostless and inferred as POSIX. + for (path, base, expected) in [ + ( + "file:///home/alice/project/src/a%20file.rs", + "file:///home/alice/project", + Some("src/a file.rs"), + ), + ( + "file:///c:/Users/Alice/project/src/main.rs", + "file:///C:/Users/Alice/project", + Some(r"src\main.rs"), + ), + ( + "file://server/share/project/src/main.rs", + "file://server/share/project", + Some(r"src\main.rs"), + ), + ( + "file:///home/alice/project", + "file:///home/alice/project/", + Some(""), + ), + ( + "file:///home/alice/project-two/main.rs", + "file:///home/alice/project", + None, + ), + ( + "file://other/share/project/main.rs", + "file://server/share/project", + None, + ), + ( + "file:///home/alice/project/src%2Fmain.rs", + "file:///home/alice/project", + None, + ), + ( + "file:///C:/project/src%5Cmain.rs", + "file:///C:/project", + None, + ), + ("file:///C:/project/main.rs", "file:///", None), + ] { + let path = PathUri::parse(path).expect("valid path URI"); + let base = PathUri::parse(base).expect("valid base URI"); + + assert_eq!( + path.relative_path_from(&base).as_deref(), + expected, + "finding {path} relative to {base}" + ); + } +} + +#[test] +fn relative_path_from_treats_fallback_uris_as_opaque() { + let path = PathUri::parse("file:///%00/bad/path/YQ").expect("valid fallback URI"); + let other = PathUri::parse("file:///%00/bad/path/Yg").expect("valid fallback URI"); + let root = PathUri::parse("file:///").expect("valid root URI"); + + assert_eq!(path.relative_path_from(&path), Some(String::new())); + assert_eq!(path.relative_path_from(&other), None); + assert_eq!(path.relative_path_from(&root), None); +} + #[cfg(windows)] #[test] fn file_uri_falls_back_for_windows_prefixes_without_a_uri_representation() {