Repository navigation
Conversation
|
@jubjub727 is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR normalizes Windows-specific path formats in two crates: ChangesWindows Path Normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant NativeVfs as NativeVfs::read_many
participant Normalizer as Path normalizers
participant Walker as walk_dir_native
participant Converter as vfs_path_from_native_path
Caller->>NativeVfs: provide glob pattern
NativeVfs->>Normalizer: normalize glob
Normalizer-->>NativeVfs: normalized glob and base directory
NativeVfs->>Walker: walk filesystem
Walker->>Normalizer: normalize candidate path
Walker->>Walker: match normalized path
Walker->>Converter: convert matched path
Converter-->>Walker: return VFS path
Walker-->>Caller: return matched entries
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/crates/bex_project/src/fs.rs`:
- Around line 171-195: The Windows drive normalization currently preserves the
original drive-letter casing, so the two VFS path producers can still create
distinct VfsPath entries for the same path. Update normalize_windows_drive_path
and baml_lsp_server/src/native_vfs.rs::vfs_path_from_native_path to normalize
the drive letter casing consistently (for example, lowercase it) before
constructing the path, or factor this logic into a shared helper used by both
code paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c015cc76-973e-478b-8b46-71f7870f7c72
📒 Files selected for processing (2)
baml_language/crates/baml_lsp_server/src/native_vfs.rsbaml_language/crates/bex_project/src/fs.rs
| #[cfg(windows)] | ||
| fn normalize_windows_drive_path(path: &std::path::Path) -> Cow<'_, std::path::Path> { | ||
| let path_as_str = path.to_string_lossy(); | ||
| let slash_path = path_as_str.replace('\\', "/"); | ||
| let Some(stripped) = slash_path.strip_prefix('/') else { | ||
| return Cow::Borrowed(path); | ||
| }; | ||
|
|
||
| if is_windows_drive_path(stripped) { | ||
| Cow::Owned(std::path::PathBuf::from(stripped)) | ||
| } else { | ||
| Cow::Borrowed(path) | ||
| } | ||
| } | ||
|
|
||
| #[cfg(not(windows))] | ||
| fn normalize_windows_drive_path(path: &std::path::Path) -> Cow<'_, std::path::Path> { | ||
| Cow::Borrowed(path) | ||
| } | ||
|
|
||
| #[cfg(windows)] | ||
| fn is_windows_drive_path(path: &str) -> bool { | ||
| let bytes = path.as_bytes(); | ||
| bytes.len() >= 3 && bytes[0].is_ascii_alphabetic() && bytes[1] == b':' && bytes[2] == b'/' | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find where VFS paths from URIs and from native discovery are compared/keyed,
# to check drive-letter case handling.
rg -nP --type=rust -C3 'to_file_path|get_path_from_uri|VfsPath|as_str\(\)' \
baml_language/crates/bex_project/src baml_language/crates/baml_lsp_server/src | head -80Repository: BoundaryML/baml
Length of output: 8428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant Rust files and inspect the path-handling functions.
git ls-files 'baml_language/crates/bex_project/src/*.rs' | rg 'fs\.rs|native_vfs\.rs|multi_project|vfs'
echo '--- fs.rs outline ---'
ast-grep outline baml_language/crates/bex_project/src/fs.rs --view expanded || true
echo '--- native_vfs.rs outline ---'
ast-grep outline baml_language/crates/bex_project/src/native_vfs.rs --view expanded || true
echo '--- fs.rs relevant slice ---'
sed -n '1,260p' baml_language/crates/bex_project/src/fs.rs | cat -n | sed -n '1,260p'
echo '--- native_vfs.rs relevant slice ---'
sed -n '1,260p' baml_language/crates/bex_project/src/native_vfs.rs | cat -n | sed -n '1,260p'Repository: BoundaryML/baml
Length of output: 13892
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the exact VFS path conversion and any string normalization helpers.
rg -n --no-heading -C 3 'vfs_path_from_native_path|get_path_from_path|normalize_windows_drive_path|is_windows_drive_path|to_string_lossy|replace\(\'\\\\\'|strip_prefix\(\'/\'\)|as_str\(\)' \
baml_language/crates/bex_project/srcRepository: BoundaryML/baml
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact implementations and any callers that key by VfsPath.
sed -n '1,240p' baml_language/crates/bex_project/src/fs.rs | cat -n
printf '\n====\n'
sed -n '1,260p' baml_language/crates/bex_project/src/native_vfs.rs | cat -n
printf '\n====\n'
rg -n --no-heading -C 2 'VfsPath::|\.as_str\(\)|dedup|sort_by|get_or_create_project|walk_dir_native|vfs_path_from_native_path|get_path_from_uri|get_path_from_path' baml_language/crates/bex_project/srcRepository: BoundaryML/baml
Length of output: 9219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the native VFS producer and read just its implementation.
rg -n --no-heading 'fn walk_dir_native|fn vfs_path_from_native_path|walk_dir_native|vfs_path_from_native_path' baml_language/crates/bex_project/srcRepository: BoundaryML/baml
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the native VFS producer and its path conversion helpers with line numbers.
if [ -f baml_language/crates/bex_project/src/native_vfs.rs ]; then
sed -n '1,260p' baml_language/crates/bex_project/src/native_vfs.rs | cat -n
else
echo "native_vfs.rs not found"
rg -n --no-heading 'walk_dir_native|vfs_path_from_native_path' baml_language/crates/bex_project/src
fiRepository: BoundaryML/baml
Length of output: 177
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the native path producer(s) and any Windows-drive normalization across the repo.
rg -n --no-heading -C 3 \
'walk_dir_native|vfs_path_from_native_path|get_path_from_path\(|normalize_windows_drive_path|is_windows_drive_path|to_file_path\(|from_path\(|VfsPath::from\(' \
baml_languageRepository: BoundaryML/baml
Length of output: 22344
Normalize the drive letter in both VFS path producers. normalize_windows_drive_path and baml_lsp_server/src/native_vfs.rs::vfs_path_from_native_path both preserve the incoming casing, so /d:/... and /D:/... can still land in different VfsPath entries on Windows. Lowercase the drive before joining, or share one helper between the two paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baml_language/crates/bex_project/src/fs.rs` around lines 171 - 195, The
Windows drive normalization currently preserves the original drive-letter
casing, so the two VFS path producers can still create distinct VfsPath entries
for the same path. Update normalize_windows_drive_path and
baml_lsp_server/src/native_vfs.rs::vfs_path_from_native_path to normalize the
drive letter casing consistently (for example, lowercase it) before constructing
the path, or factor this logic into a shared helper used by both code paths.
sxlijin
left a comment
sxlijin
left a comment
There was a problem hiding this comment.
Ahh, so vfs requires Unix-style paths. That's annoying - doing a quick skim it looks like https://docs.rs/path-slash/latest/path_slash/ might be the best crate to use for this?
I don't love the idea of reimplementing windows path normalization multiple times; I'd rather have a new crate baml_language/crates/utils_windows/ for that purpose (will need a cargo-stow update to make utils_ allowed as well).
|
Absolutely agree. The only reason I made this fix is because I wanted to get it working on my machine so I could go back to working on the project I'm making with BAML. I mainly made this PR as a way of communicating the specific behaviour I was experiencing and the potential root cause behind it rather than as a serious long term proposal for fixing these path bugs. I'm also very thankful that you were able to review this and find a relevant library here as that gives me a very easy starting point and clear direction to go from. As I mentioned in discord I think a more detailed dive into not just this specific path handling but instead how paths are handled throughout the entire project including the various layers not just Rust is the most appropriate path forward. I'm happy to go off, figure this all out and come back to you with a detailed report of my findings and how I'm thinking of solving it more widely if that sounds good to you. As much as I feel like I'm perfectly capable of solving a task of this scope on my own I am also cognizant that I'm unfamiliar with the nuances of the code base and how that can lead to accidentally creating a bunch of work for your team later on. Programming languages are hard and deserve patience, respect and discipline imo :) |
b94a38d to
f0a3833
Compare
|
Closing this now. The better solution was included in the first batch of fixes here: #4103 |
Summary
.bamlfiles from disk.Codex Comment
The bug was caused by Windows paths crossing between three path formats: native paths like
D:\..., URI/VFS-style paths like/d:/..., and mixed forms like\d:\.... BAML's VFS path logic uses/as its separator, so raw backslash paths could not walk to parent directories correctly. Separately,/d:/...could be treated as relative and joined against the server cwd, producing duplicated paths like/d:/d:/....The playground/LSP source loader also built VFS-style globs such as
/d:/project/baml_src/**/*.baml, then passed them directly into native filesystem matching on Windows. That meant sibling files likeclients.bamlcould be skipped, leaving only the open in-memory file loaded. In practice, valid client declarations appeared as unresolved names because their defining file was absent from the project view.This fix keeps the translation at the filesystem/VFS boundary: Windows-only normalization before VFS conversion, and Windows-only conversion between VFS drive globs and native disk paths during source discovery. Non-Windows behavior is left unchanged.
Author Comment
I used Codex to investigate the root cause of these issues. Its understanding of the issue looks accurate on the surface and the fix appears to work on my machine. I haven't manually confirmed the actual root cause analysis myself but its work looks good to me.
Summary by CodeRabbit
/handling).