From d9eb186b880c7ab4bcd4e4d7ee10aac3f66d2325 Mon Sep 17 00:00:00 2001 From: Shane Wall <214650244+saworbit@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:39:14 +1100 Subject: [PATCH 1/3] Refuse trailing-separator destination and normalize path spelling in errors Reject destinations that end with a path separator with exit code 3 and code destination_has_trailing_separator. The raw input is inspected before Windows GetFullPathNameW drops the trailing character, so copy and plan behave the same way on both platforms. Strip reversible \?\C:\ and \?\UNC\ prefixes in display_path on Windows so human-mode errors use the same ordinary spelling regardless of which arm produced them. Fixes #110 Fixes #111 --- CHANGELOG.md | 17 ++++++++++ src/error.rs | 4 +++ src/paths.rs | 41 +++++++++++++++++++++++ src/report.rs | 68 +++++++++++++++++++++++++++++++++++++++ tests/copy_integration.rs | 45 ++++++++++++++++++++++++++ tests/plan_integration.rs | 32 ++++++++++++++++-- 6 files changed, 205 insertions(+), 2 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 0000000..5bb5fb8 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,17 @@ +# Changelog + +All notable changes to Orbit are recorded here. The format follows +[Keep a Changelog](https://keepachangelog.com/en/1.1.0/) and this project +adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +### Fixed + +- `orbit copy` and `orbit plan` now refuse a destination that ends with a + path separator (exit code 3, `destination_has_trailing_separator`). A + trailing separator is a directory signal, so silently normalizing it to a + single-file destination was surprising. Fixes #110. +- Windows error messages render paths with a consistent ordinary spelling. + Reversible `\\?\C:\` and `\\?\UNC\` prefixes are now stripped before + display so similar errors read the same way. Fixes #111. diff --git a/src/error.rs b/src/error.rs index a998788..a288e51 100644 --- a/src/error.rs +++ b/src/error.rs @@ -15,6 +15,8 @@ pub enum OrbitError { DestinationExists(PathBuf), #[error("destination parent does not exist or is not a directory: {0}")] DestinationParentMissing(PathBuf), + #[error("destination has a trailing path separator, expected a single file path: {0}")] + DestinationHasTrailingSeparator(PathBuf), #[error("staged copy did not match the source: {0}")] VerificationFailed(PathBuf), #[error("source changed while it was being copied: {0}")] @@ -45,6 +47,7 @@ impl OrbitError { Self::CopyRequiresRegularFile(_) => "copy_requires_regular_file", Self::DestinationExists(_) => "destination_exists", Self::DestinationParentMissing(_) => "destination_parent_missing", + Self::DestinationHasTrailingSeparator(_) => "destination_has_trailing_separator", Self::VerificationFailed(_) => "verification_failed", Self::SourceChanged(_) => "source_changed", Self::StagingChanged(_) => "staging_changed", @@ -61,6 +64,7 @@ impl OrbitError { | Self::CopyRequiresRegularFile(_) | Self::DestinationExists(_) | Self::DestinationParentMissing(_) + | Self::DestinationHasTrailingSeparator(_) | Self::SourceChanged(_) => 3, Self::VerificationFailed(_) | Self::StagingChanged(_) | Self::StagingCleanup { .. } => { 1 diff --git a/src/paths.rs b/src/paths.rs index ae2a49d..5c4afed 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -20,6 +20,11 @@ pub fn resolve_endpoints( destination: &Path, cwd: &Path, ) -> Result { + if ends_with_separator(destination) { + return Err(OrbitError::DestinationHasTrailingSeparator( + destination.to_path_buf(), + )); + } let source_input = absolutize(source, cwd)?; let source_metadata = match fs::symlink_metadata(&source_input) { Ok(metadata) => metadata, @@ -49,6 +54,22 @@ pub fn resolve_endpoints( }) } +#[cfg(windows)] +fn ends_with_separator(path: &Path) -> bool { + // The raw user input must be inspected here because GetFullPathNameW drops + // trailing separators before resolve_leaf can see them. + path.as_os_str() + .encode_wide() + .last() + .is_some_and(|unit| unit == u16::from(b'\\') || unit == u16::from(b'/')) +} + +#[cfg(not(windows))] +fn ends_with_separator(path: &Path) -> bool { + use std::os::unix::ffi::OsStrExt; + matches!(path.as_os_str().as_bytes().last(), Some(&b'/')) +} + fn absolutize(path: &Path, cwd: &Path) -> Result { let path = if path.is_absolute() { path.to_path_buf() @@ -287,6 +308,26 @@ mod tests { )); } + #[test] + fn rejects_destination_with_trailing_separator() { + let temp = tempdir().unwrap(); + let source = temp.path().join("source.txt"); + fs::write(&source, b"source").unwrap(); + for suffix in ["new_dir/", "new_dir\\"] { + let mut raw = temp.path().as_os_str().to_os_string(); + raw.push(std::path::MAIN_SEPARATOR_STR); + raw.push(suffix); + let destination = std::path::PathBuf::from(raw); + assert!( + matches!( + resolve_endpoints(&source, &destination, temp.path()), + Err(OrbitError::DestinationHasTrailingSeparator(_)) + ), + "destination {destination:?} slipped through trailing-separator check" + ); + } + } + #[test] fn rejects_destination_inside_source_directory() { let temp = tempdir().unwrap(); diff --git a/src/report.rs b/src/report.rs index c89da0b..062c590 100644 --- a/src/report.rs +++ b/src/report.rs @@ -186,6 +186,10 @@ fn error_message(error: &OrbitError) -> String { "destination parent does not exist or is not a directory: {}", display_path(path) ), + OrbitError::DestinationHasTrailingSeparator(path) => format!( + "destination has a trailing path separator, expected a single file path: {}", + display_path(path) + ), OrbitError::VerificationFailed(path) => format!( "staged copy did not match the source: {}", display_path(path) @@ -304,6 +308,7 @@ fn write_json_line(writer: &mut impl Write, value: &impl serde::Serialize) -> io writeln!(writer) } +#[cfg(not(windows))] fn display_path(path: &Path) -> String { match path.to_str() { Some(value) => value.to_owned(), @@ -311,6 +316,30 @@ fn display_path(path: &Path) -> String { } } +#[cfg(windows)] +fn display_path(path: &Path) -> String { + let Some(value) = path.to_str() else { + return format!("[{}:{}]", native_path_encoding(), native_path_hex(path)); + }; + let Some(rest) = value.strip_prefix(r"\\?\") else { + return value.to_owned(); + }; + if let Some(unc) = rest.strip_prefix("UNC\\") { + return format!(r"\\{unc}"); + } + let bytes = rest.as_bytes(); + let drive_letter = bytes.first().copied().is_some_and(|c| c.is_ascii_alphabetic()); + let drive_colon = bytes.get(1).copied() == Some(b':'); + let drive_root = bytes + .get(2) + .copied() + .is_none_or(|c| c == b'\\' || c == b'/'); + if drive_letter && drive_colon && drive_root { + return rest.to_owned(); + } + value.to_owned() +} + #[cfg(unix)] fn native_path_encoding() -> &'static str { "unix_bytes_hex" @@ -510,6 +539,45 @@ mod tests { ); } + #[cfg(windows)] + #[test] + fn display_path_strips_reversible_verbatim_prefix() { + use super::display_path; + use std::path::PathBuf; + + assert_eq!( + display_path(&PathBuf::from(r"\\?\C:\tmp\vibe\noexist")), + r"C:\tmp\vibe\noexist" + ); + assert_eq!(display_path(&PathBuf::from(r"\\?\C:\")), r"C:\"); + assert_eq!( + display_path(&PathBuf::from(r"\\?\UNC\server\share\file.txt")), + r"\\server\share\file.txt" + ); + // Non-reversible verbatim forms stay as the operating system reported them. + let volume = r"\\?\Volume{00000000-0000-0000-0000-000000000000}\file.txt"; + assert_eq!(display_path(&PathBuf::from(volume)), volume); + assert_eq!( + display_path(&PathBuf::from(r"C:\tmp\vibe\missing.txt")), + r"C:\tmp\vibe\missing.txt" + ); + } + + #[test] + fn trailing_separator_error_message_names_the_destination() { + use super::error_message; + use std::path::PathBuf; + + let message = error_message(&OrbitError::DestinationHasTrailingSeparator( + PathBuf::from("out/"), + )); + assert!( + message.contains("trailing path separator"), + "message was: {message}" + ); + assert!(message.contains("out/"), "message was: {message}"); + } + #[test] fn every_error_variant_has_a_stable_code_and_exit_code() { let errors = [ diff --git a/tests/copy_integration.rs b/tests/copy_integration.rs index 1fdb3e8..65a6440 100644 --- a/tests/copy_integration.rs +++ b/tests/copy_integration.rs @@ -128,6 +128,28 @@ fn missing_destination_parent_is_refused_without_creating_directories() { assert!(partials(temp.path()).is_empty()); } +#[test] +fn trailing_separator_destination_is_refused_without_creating_a_file() { + let temp = tempfile::tempdir().unwrap(); + fs::write(temp.path().join("source.bin"), b"payload").unwrap(); + + #[cfg(windows)] + let destinations = ["new_dir/", "new_dir\\"]; + #[cfg(not(windows))] + let destinations = ["new_dir/"]; + + for destination in destinations { + orbit(temp.path()) + .args(["copy", "source.bin", destination]) + .assert() + .code(3) + .stderr(predicate::str::contains("trailing path separator")); + + assert!(!temp.path().join("new_dir").exists()); + assert!(partials(temp.path()).is_empty()); + } +} + #[test] fn json_success_is_one_machine_readable_result() { let temp = tempfile::tempdir().unwrap(); @@ -185,6 +207,29 @@ fn quiet_refusal_remains_visible_on_stderr() { .stderr(predicate::str::contains("destination already exists")); } +#[cfg(windows)] +#[test] +fn error_messages_use_ordinary_path_spelling_on_windows() { + let temp = tempfile::tempdir().unwrap(); + fs::write(temp.path().join("source.bin"), b"payload").unwrap(); + + let output = orbit(temp.path()) + .args(["copy", "source.bin", "missing/destination.bin"]) + .output() + .unwrap(); + + assert_eq!(output.status.code(), Some(3)); + let stderr = String::from_utf8(output.stderr).unwrap(); + assert!( + !stderr.contains(r"\\?\"), + "error message leaked verbatim prefix: {stderr}" + ); + assert!( + stderr.contains("destination parent"), + "unexpected message: {stderr}" + ); +} + #[test] fn symbolic_link_source_is_refused_without_following_it() { let temp = tempfile::tempdir().unwrap(); diff --git a/tests/plan_integration.rs b/tests/plan_integration.rs index 19c6c31..cd7fdf1 100644 --- a/tests/plan_integration.rs +++ b/tests/plan_integration.rs @@ -426,13 +426,41 @@ fn quiet_conflict_reports_exact_blocking_paths_and_reasons_on_stderr() { String::from_utf8(output.stderr).unwrap(), format!( "error: CONFLICT {} — destination file has different contents\nerror: CONFLICT {} — destination file has different contents\n", - resolved_destination.join("a.txt").to_str().unwrap(), - resolved_destination.join("b.txt").to_str().unwrap(), + display_path(&resolved_destination.join("a.txt")), + display_path(&resolved_destination.join("b.txt")), ) ); assert_tree_unchanged(temp.path(), &before); } +/// Mirrors `report::display_path` on Windows: reversible `\\?\` prefixes are +/// stripped so human error output matches an ordinary path spelling. +fn display_path(path: &Path) -> String { + let value = path.to_str().expect("test paths are UTF-8"); + #[cfg(windows)] + { + if let Some(rest) = value.strip_prefix(r"\\?\") { + if let Some(unc) = rest.strip_prefix("UNC\\") { + return format!(r"\\{unc}"); + } + let bytes = rest.as_bytes(); + let drive_letter = bytes + .first() + .copied() + .is_some_and(|c| c.is_ascii_alphabetic()); + let drive_colon = bytes.get(1).copied() == Some(b':'); + let drive_root = bytes + .get(2) + .copied() + .is_none_or(|c| c == b'\\' || c == b'/'); + if drive_letter && drive_colon && drive_root { + return rest.to_owned(); + } + } + } + value.to_owned() +} + #[cfg(unix)] #[test] fn quiet_unsupported_socket_reports_source_and_can_be_explicitly_ignored() { From 29c0f6db5bb1843eef4260e1ff91cd033f6979f4 Mon Sep 17 00:00:00 2001 From: Shane Wall <214650244+saworbit@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:37:48 +1100 Subject: [PATCH 2/3] Apply rustfmt to report.rs --- src/report.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/report.rs b/src/report.rs index 062c590..0147b20 100644 --- a/src/report.rs +++ b/src/report.rs @@ -328,7 +328,10 @@ fn display_path(path: &Path) -> String { return format!(r"\\{unc}"); } let bytes = rest.as_bytes(); - let drive_letter = bytes.first().copied().is_some_and(|c| c.is_ascii_alphabetic()); + let drive_letter = bytes + .first() + .copied() + .is_some_and(|c| c.is_ascii_alphabetic()); let drive_colon = bytes.get(1).copied() == Some(b':'); let drive_root = bytes .get(2) @@ -568,9 +571,9 @@ mod tests { use super::error_message; use std::path::PathBuf; - let message = error_message(&OrbitError::DestinationHasTrailingSeparator( - PathBuf::from("out/"), - )); + let message = error_message(&OrbitError::DestinationHasTrailingSeparator(PathBuf::from( + "out/", + ))); assert!( message.contains("trailing path separator"), "message was: {message}" From ef9bb46ccf9c3d123e95bc5a535ece240904de15 Mon Sep 17 00:00:00 2001 From: Shane Wall <214650244+saworbit@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:41:15 +1100 Subject: [PATCH 3/3] Scope trailing-separator test to host separators On Unix a backslash is a literal filename character, not a path separator, so the Unix branch only exercises "/" suffixes while Windows still exercises both "/" and "\\". The production check already behaves this way; only the unit test iterated both separators unconditionally. --- src/paths.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/paths.rs b/src/paths.rs index 5c4afed..9a3f76c 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -313,7 +313,14 @@ mod tests { let temp = tempdir().unwrap(); let source = temp.path().join("source.txt"); fs::write(&source, b"source").unwrap(); - for suffix in ["new_dir/", "new_dir\\"] { + // `\\` is a path separator on Windows but a literal filename character + // on Unix, so only check the separators the host actually uses. + let suffixes: &[&str] = if cfg!(windows) { + &["new_dir/", "new_dir\\"] + } else { + &["new_dir/"] + }; + for suffix in suffixes { let mut raw = temp.path().as_os_str().to_os_string(); raw.push(std::path::MAIN_SEPARATOR_STR); raw.push(suffix);