codex: address PR review feedback (#27819)

This commit is contained in:
Adam Perry
2026-06-13 03:01:39 +00:00
committed by Adam Perry @ OpenAI
parent 7a60009b73
commit 7b359240df
2 changed files with 332 additions and 221 deletions

View File

@@ -56,6 +56,12 @@ pub struct NativePathString(String);
impl NativePathString {
/// Renders a path URI using the requested native path convention.
///
/// Rendering fails when the URI shape does not match the convention, such
/// as a POSIX path rendered as Windows or a UNC path rendered as POSIX. It
/// also fails when an opaque fallback does not encode an absolute path for
/// the convention, when a URI segment is not valid UTF-8, or when a segment
/// contains a percent-encoded separator that would change path structure.
pub fn from_path_uri(
path: &PathUri,
convention: PathConvention,
@@ -63,11 +69,11 @@ impl NativePathString {
if let Some(path_bytes) = path.opaque_fallback_bytes() {
return render_opaque_fallback(path, &path_bytes, convention).map(Self);
}
let value = match convention {
PathConvention::Posix => render_posix_path(path)?,
PathConvention::Windows => render_windows_path(path)?,
};
Ok(Self(value))
match convention {
PathConvention::Posix => render_posix_path(path),
PathConvention::Windows => render_windows_path(path),
}
.map(Self)
}
pub fn as_str(&self) -> &str {

View File

@@ -2,183 +2,341 @@ use super::*;
use crate::PathUri;
use pretty_assertions::assert_eq;
#[test]
fn renders_posix_paths_on_every_host() {
for (uri, expected) in [
("file:///", "/"),
("file:///home/alice/src/main.rs", "/home/alice/src/main.rs"),
("file:///home/alice/a%20file.rs", "/home/alice/a file.rs"),
("file:///workspace/src/lib.rs", "/workspace/src/lib.rs"),
(
"file:///workspace/tests/test.rs",
"/workspace/tests/test.rs",
),
("file:///etc", "/etc"),
("file:///tmp/", "/tmp/"),
("file:///C:/Project", "/C:/Project"),
("file:///C:", "/C:"),
("file:///tmp/%E2%98%83", "/tmp/☃"),
("file:///tmp/a%5Cb", "/tmp/a\\b"),
("file:///tmp/100%25/file", "/tmp/100%/file"),
("file:///tmp/a%3Fb%23c%25d", "/tmp/a?b#c%d"),
("file:///tmp/a%252Fb", "/tmp/a%2Fb"),
(
"file:///bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
"/bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
),
("FILE:///workspace/src", "/workspace/src"),
("file:/workspace/src", "/workspace/src"),
("file://localhost/workspace/src", "/workspace/src"),
("file://LOCALHOST/workspace/src", "/workspace/src"),
] {
let path = PathUri::parse(uri).expect("valid file URI");
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Posix)
.map(NativePathString::into_string),
Ok(expected.to_string()),
"rendering {uri}"
);
#[derive(Clone, Copy, Debug)]
struct RenderCase {
uri: &'static str,
convention: PathConvention,
expected: RenderExpectation,
}
impl RenderCase {
const fn renders(
uri: &'static str,
convention: PathConvention,
rendered: &'static str,
) -> Self {
Self {
uri,
convention,
expected: RenderExpectation::Rendered(rendered),
}
}
const fn rejects(uri: &'static str, convention: PathConvention, error: ExpectedError) -> Self {
Self {
uri,
convention,
expected: RenderExpectation::Error(error),
}
}
}
#[test]
fn renders_windows_drive_paths_on_every_host() {
for (uri, expected) in [
(
"file:///C:/Users/Alice%20Smith/src/main.rs",
r"C:\Users\Alice Smith\src\main.rs",
),
("file:///C:/", "C:\\"),
("file:///C:", "C:\\"),
("file:///C:/Users", r"C:\Users"),
("file:///C:/Windows", r"C:\Windows"),
("file:///d:/snowman/%E2%98%83", r"d:\snowman\☃"),
("file:///C:/tmp/", "C:\\tmp\\"),
("file:///C:/test%20with%20%25/path", r"C:\test with %\path"),
(
"file:///C:/test%20with%20%2525/c%23code",
r"C:\test with %25\c#code",
),
(
"file:///C:/Source/Z%C3%BCrich%20or%20Zurich%20(%CB%88zj%CA%8A%C9%99r%C9%AAk,/Code/resources/app/plugins/c%23/plugin.json",
r"C:\Source\Zürich or Zurich (ˈzjʊərɪk,\Code\resources\app\plugins\c#\plugin.json",
),
(
"file:///C:/Users/Abd-al-Haseeb%27s_Dell/Studio/w3mage/wp-content/database.ht.sqlite",
r"C:\Users\Abd-al-Haseeb's_Dell\Studio\w3mage\wp-content\database.ht.sqlite",
),
("file:///C:/project/%25A0.txt", r"C:\project\%A0.txt"),
("file:///C:/project/%252e.txt", r"C:\project\%2e.txt"),
] {
let path = PathUri::parse(uri).expect("valid file URI");
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Windows)
.map(NativePathString::into_string),
Ok(expected.to_string()),
"rendering {uri}"
);
}
#[derive(Clone, Copy, Debug)]
enum RenderExpectation {
Rendered(&'static str),
Error(ExpectedError),
}
#[test]
fn renders_windows_unc_paths_on_every_host() {
for (uri, expected) in [
(
"file://server/share/src/main.rs",
r"\\server\share\src\main.rs",
),
("file://server/share", r"\\server\share"),
("file://server/share/", "\\\\server\\share\\"),
("file://shares/files/c%23/p.cs", r"\\shares\files\c#\p.cs"),
(
"file://monacotools1/certificates/SSL/",
"\\\\monacotools1\\certificates\\SSL\\",
),
] {
let path = PathUri::parse(uri).expect("valid file URI");
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Windows)
.map(NativePathString::into_string),
Ok(expected.to_string()),
"rendering {uri}"
);
}
#[derive(Clone, Copy, Debug)]
enum ExpectedError {
OpaqueFallback,
IncompatibleConvention,
NonUtf8,
EncodedSeparator,
}
#[test]
fn rejects_paths_incompatible_with_the_convention() {
for (uri, convention) in [
("file://server/share/file.txt", PathConvention::Posix),
("file://server/share/file.rs", PathConvention::Posix),
("file:///usr/local/file.txt", PathConvention::Windows),
("file:///home/alice/file.rs", PathConvention::Windows),
("file://server/", PathConvention::Windows),
("file:///_:/path", PathConvention::Windows),
] {
let path = PathUri::parse(uri).expect("valid file URI");
assert!(matches!(
NativePathString::from_path_uri(&path, convention),
Err(NativePathStringError::IncompatibleConvention { .. })
));
}
}
const RENDER_CASES: &[RenderCase] = &[
// POSIX paths.
RenderCase::renders("file:///", PathConvention::Posix, "/"),
RenderCase::renders(
"file:///home/alice/src/main.rs",
PathConvention::Posix,
"/home/alice/src/main.rs",
),
RenderCase::renders(
"file:///home/alice/a%20file.rs",
PathConvention::Posix,
"/home/alice/a file.rs",
),
RenderCase::renders(
"file:///workspace/src/lib.rs",
PathConvention::Posix,
"/workspace/src/lib.rs",
),
RenderCase::renders(
"file:///workspace/tests/test.rs",
PathConvention::Posix,
"/workspace/tests/test.rs",
),
RenderCase::renders("file:///etc", PathConvention::Posix, "/etc"),
RenderCase::renders("file:///tmp/", PathConvention::Posix, "/tmp/"),
RenderCase::renders("file:///C:/Project", PathConvention::Posix, "/C:/Project"),
RenderCase::renders("file:///C:", PathConvention::Posix, "/C:"),
RenderCase::renders("file:///tmp/%E2%98%83", PathConvention::Posix, "/tmp/☃"),
RenderCase::renders("file:///tmp/a%5Cb", PathConvention::Posix, "/tmp/a\\b"),
RenderCase::renders(
"file:///tmp/100%25/file",
PathConvention::Posix,
"/tmp/100%/file",
),
RenderCase::renders(
"file:///tmp/a%3Fb%23c%25d",
PathConvention::Posix,
"/tmp/a?b#c%d",
),
RenderCase::renders("file:///tmp/a%252Fb", PathConvention::Posix, "/tmp/a%2Fb"),
RenderCase::renders(
"file:///bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
PathConvention::Posix,
"/bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
),
RenderCase::renders(
"FILE:///workspace/src",
PathConvention::Posix,
"/workspace/src",
),
RenderCase::renders(
"file:/workspace/src",
PathConvention::Posix,
"/workspace/src",
),
RenderCase::renders(
"file://localhost/workspace/src",
PathConvention::Posix,
"/workspace/src",
),
RenderCase::renders(
"file://LOCALHOST/workspace/src",
PathConvention::Posix,
"/workspace/src",
),
// Windows drive paths.
RenderCase::renders(
"file:///C:/Users/Alice%20Smith/src/main.rs",
PathConvention::Windows,
r"C:\Users\Alice Smith\src\main.rs",
),
RenderCase::renders("file:///C:/", PathConvention::Windows, "C:\\"),
RenderCase::renders("file:///C:", PathConvention::Windows, "C:\\"),
RenderCase::renders("file:///C:/Users", PathConvention::Windows, r"C:\Users"),
RenderCase::renders("file:///C:/Windows", PathConvention::Windows, r"C:\Windows"),
RenderCase::renders(
"file:///d:/snowman/%E2%98%83",
PathConvention::Windows,
r"d:\snowman\☃",
),
RenderCase::renders("file:///C:/tmp/", PathConvention::Windows, "C:\\tmp\\"),
RenderCase::renders(
"file:///C:/test%20with%20%25/path",
PathConvention::Windows,
r"C:\test with %\path",
),
RenderCase::renders(
"file:///C:/test%20with%20%2525/c%23code",
PathConvention::Windows,
r"C:\test with %25\c#code",
),
RenderCase::renders(
"file:///C:/Source/Z%C3%BCrich%20or%20Zurich%20(%CB%88zj%CA%8A%C9%99r%C9%AAk,/Code/resources/app/plugins/c%23/plugin.json",
PathConvention::Windows,
r"C:\Source\Zürich or Zurich (ˈzjʊərɪk,\Code\resources\app\plugins\c#\plugin.json",
),
RenderCase::renders(
"file:///C:/Users/Abd-al-Haseeb%27s_Dell/Studio/w3mage/wp-content/database.ht.sqlite",
PathConvention::Windows,
r"C:\Users\Abd-al-Haseeb's_Dell\Studio\w3mage\wp-content\database.ht.sqlite",
),
RenderCase::renders(
"file:///C:/project/%25A0.txt",
PathConvention::Windows,
r"C:\project\%A0.txt",
),
RenderCase::renders(
"file:///C:/project/%252e.txt",
PathConvention::Windows,
r"C:\project\%2e.txt",
),
// Windows UNC paths.
RenderCase::renders(
"file://server/share/src/main.rs",
PathConvention::Windows,
r"\\server\share\src\main.rs",
),
RenderCase::renders(
"file://server/share",
PathConvention::Windows,
r"\\server\share",
),
RenderCase::renders(
"file://server/share/",
PathConvention::Windows,
"\\\\server\\share\\",
),
RenderCase::renders(
"file://shares/files/c%23/p.cs",
PathConvention::Windows,
r"\\shares\files\c#\p.cs",
),
RenderCase::renders(
"file://monacotools1/certificates/SSL/",
PathConvention::Windows,
"\\\\monacotools1\\certificates\\SSL\\",
),
// Opaque fallbacks rendered according to their source convention.
RenderCase::renders(
"file:///%00/bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
PathConvention::Posix,
"/tmp/null-\0-<2D>-byte",
),
RenderCase::renders(
"file:///%00/bad/path/XABcAC4AXABDAE8ATQAxAFwA",
PathConvention::Windows,
r"\\.\COM1\",
),
RenderCase::renders(
"file:///%00/bad/path/XABcAD8AXABWAG8AbAB1AG0AZQB7ADAAMAAwADAAMAAwADAAMAAtADAAMAAwADAALQAwADAAMAAwAC0AMAAwADAAMAAtADAAMAAwADAAMAAwADAAMAAwADAAMAAwAH0AXABmAGkAbABlAC4AcgBzAA",
PathConvention::Windows,
r"\\?\Volume{00000000-0000-0000-0000-000000000000}\file.rs",
),
// Windows rendering preserves path text without filesystem validation.
RenderCase::renders("file:///C:/a%3Fb", PathConvention::Windows, "C:\\a?b"),
RenderCase::renders("file:///C:/a%2Ab", PathConvention::Windows, "C:\\a*b"),
RenderCase::renders(
"file:///C:/trailing.",
PathConvention::Windows,
"C:\\trailing.",
),
RenderCase::renders(
"file:///C:/trailing%20",
PathConvention::Windows,
"C:\\trailing ",
),
RenderCase::renders(
"file:///C:/control-%01",
PathConvention::Windows,
"C:\\control-\u{1}",
),
RenderCase::renders(
"file:///C:/file.txt:stream",
PathConvention::Windows,
"C:\\file.txt:stream",
),
RenderCase::renders(
"file://server/sh%3Fare/file.rs",
PathConvention::Windows,
"\\\\server\\sh?are\\file.rs",
),
// URI shapes that do not match the requested convention.
RenderCase::rejects(
"file://server/share/file.txt",
PathConvention::Posix,
ExpectedError::IncompatibleConvention,
),
RenderCase::rejects(
"file://server/share/file.rs",
PathConvention::Posix,
ExpectedError::IncompatibleConvention,
),
RenderCase::rejects(
"file:///usr/local/file.txt",
PathConvention::Windows,
ExpectedError::IncompatibleConvention,
),
RenderCase::rejects(
"file:///home/alice/file.rs",
PathConvention::Windows,
ExpectedError::IncompatibleConvention,
),
RenderCase::rejects(
"file://server/",
PathConvention::Windows,
ExpectedError::IncompatibleConvention,
),
RenderCase::rejects(
"file:///_:/path",
PathConvention::Windows,
ExpectedError::IncompatibleConvention,
),
// Invalid opaque fallback payloads.
RenderCase::rejects(
"file:///%00/bad/path/YQ",
PathConvention::Posix,
ExpectedError::OpaqueFallback,
),
RenderCase::rejects(
"file:///%00/bad/path/L3RtcC9udWxsLQAt_y1ieXRl",
PathConvention::Windows,
ExpectedError::OpaqueFallback,
),
// URI segment encodings that cannot be rendered without changing meaning.
RenderCase::rejects(
"file:///tmp/non-utf8-%FF",
PathConvention::Posix,
ExpectedError::NonUtf8,
),
RenderCase::rejects(
"file:///tmp/non-utf8-%A0",
PathConvention::Posix,
ExpectedError::NonUtf8,
),
RenderCase::rejects(
"file:///tmp/a%2Fb",
PathConvention::Posix,
ExpectedError::EncodedSeparator,
),
RenderCase::rejects(
"file:///C:/a%2Fb",
PathConvention::Windows,
ExpectedError::EncodedSeparator,
),
RenderCase::rejects(
"file:///C:/a%5Cb",
PathConvention::Windows,
ExpectedError::EncodedSeparator,
),
];
#[test]
fn rejects_opaque_fallback_paths_that_cannot_be_recovered() {
let path = PathUri::parse("file:///%00/bad/path/YQ").expect("canonical opaque fallback URI");
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::native()),
Err(NativePathStringError::OpaqueFallback {
path: path.to_string(),
})
);
}
#[test]
fn renders_posix_opaque_fallback_paths_lossily_on_every_host() {
let path = PathUri::parse("file:///%00/bad/path/L3RtcC9udWxsLQAt_y1ieXRl")
.expect("valid POSIX opaque fallback URI");
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Posix)
.map(NativePathString::into_string),
Ok("/tmp/null-\0-<2D>-byte".to_string())
);
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Windows),
Err(NativePathStringError::OpaqueFallback {
path: path.to_string(),
})
);
}
#[test]
fn renders_windows_namespace_fallback_paths_on_every_host() {
for (uri, expected) in [
(
"file:///%00/bad/path/XABcAC4AXABDAE8ATQAxAFwA",
r"\\.\COM1\",
),
(
"file:///%00/bad/path/XABcAD8AXABWAG8AbAB1AG0AZQB7ADAAMAAwADAAMAAwADAAMAAtADAAMAAwADAALQAwADAAMAAwAC0AMAAwADAAMAAtADAAMAAwADAAMAAwADAAMAAwADAAMAAwAH0AXABmAGkAbABlAC4AcgBzAA",
r"\\?\Volume{00000000-0000-0000-0000-000000000000}\file.rs",
),
] {
let path = PathUri::parse(uri).expect("valid Windows opaque fallback URI");
fn renders_native_paths_from_shared_cases() {
for case in RENDER_CASES {
let path = PathUri::parse(case.uri).expect("valid file URI");
let expected = match case.expected {
RenderExpectation::Rendered(rendered) => Ok(NativePathString(rendered.to_string())),
RenderExpectation::Error(ExpectedError::OpaqueFallback) => {
Err(NativePathStringError::OpaqueFallback {
path: path.to_string(),
})
}
RenderExpectation::Error(ExpectedError::IncompatibleConvention) => {
Err(NativePathStringError::IncompatibleConvention {
path: path.to_string(),
convention: case.convention,
})
}
RenderExpectation::Error(ExpectedError::NonUtf8) => {
Err(NativePathStringError::NonUtf8 {
path: path.to_string(),
})
}
RenderExpectation::Error(ExpectedError::EncodedSeparator) => {
Err(NativePathStringError::EncodedSeparator {
path: path.to_string(),
convention: case.convention,
})
}
};
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Windows)
.map(NativePathString::into_string),
Ok(expected.to_string()),
"rendering {uri}"
NativePathString::from_path_uri(&path, case.convention),
expected,
"rendering {case:?}"
);
}
}
#[cfg(windows)]
#[test]
fn renders_native_opaque_fallback_paths_lossily() {
fn renders_native_non_unicode_windows_fallback_lossily() {
use std::os::windows::ffi::OsStringExt;
let native_path = std::path::PathBuf::from(std::ffi::OsString::from_wide(
@@ -190,70 +348,17 @@ fn renders_native_opaque_fallback_paths_lossily() {
let path = PathUri::from_path(native_path).expect("absolute native path");
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Windows)
.map(NativePathString::into_string),
Ok(r"C:\bad\<5C>".to_string())
NativePathString::from_path_uri(&path, PathConvention::Windows),
Ok(NativePathString(r"C:\bad\<5C>".to_string()))
);
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Posix),
Err(NativePathStringError::IncompatibleConvention {
Err(NativePathStringError::OpaqueFallback {
path: path.to_string(),
convention: PathConvention::Posix,
})
);
}
#[test]
fn rejects_non_utf8_paths() {
for uri in ["file:///tmp/non-utf8-%FF", "file:///tmp/non-utf8-%A0"] {
let path = PathUri::parse(uri).expect("valid file URI");
assert!(matches!(
NativePathString::from_path_uri(&path, PathConvention::Posix),
Err(NativePathStringError::NonUtf8 { .. })
));
}
}
#[test]
fn rejects_encoded_separators() {
for (uri, convention) in [
("file:///tmp/a%2Fb", PathConvention::Posix),
("file:///C:/a%2Fb", PathConvention::Windows),
("file:///C:/a%5Cb", PathConvention::Windows),
] {
let path = PathUri::parse(uri).expect("valid file URI");
assert!(matches!(
NativePathString::from_path_uri(&path, convention),
Err(NativePathStringError::EncodedSeparator { .. })
));
}
}
#[test]
fn renders_windows_components_without_filesystem_validation() {
for (uri, expected) in [
("file:///C:/a%3Fb", "C:\\a?b"),
("file:///C:/a%2Ab", "C:\\a*b"),
("file:///C:/trailing.", "C:\\trailing."),
("file:///C:/trailing%20", "C:\\trailing "),
("file:///C:/control-%01", "C:\\control-\u{1}"),
("file:///C:/file.txt:stream", "C:\\file.txt:stream"),
(
"file://server/sh%3Fare/file.rs",
"\\\\server\\sh?are\\file.rs",
),
] {
let path = PathUri::parse(uri).expect("valid file URI");
assert_eq!(
NativePathString::from_path_uri(&path, PathConvention::Windows),
Ok(NativePathString(expected.to_string())),
"rendering {uri}"
);
}
}
#[test]
fn serializes_as_a_string() {
let path = PathUri::parse("file:///workspace/src/lib.rs").expect("valid file URI");