Repository navigation
fix(windows): harden path and URI handling (batch 1) - #4103
Merged
sxlijin merged 16 commits intoAug 14, 2026
Merged
Conversation
|
@jubjub727 is attempting to deploy a commit to the Boundary Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (20)
🚧 Files skipped from review as they are similar to previous changes (19)
📝 WalkthroughWalkthroughThe PR adds cross-platform native/VFS path types, ignore-aware BAML discovery, native LSP filesystem access, explicit BEX path conversion, and a URI-based workspace path model for the TypeScript editor. ChangesRust native paths and discovery
BEX native and VFS path plumbing
TypeScript editor workspace paths
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: ⚪ Minimal · up to The changes harden Windows path discovery, filesystem/VFS conversion, and editor URI handling with corresponding regression coverage; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant LSPWorkspace
participant BamlVFS
participant NativePathBuf
participant BamlDiscovery
participant NativeVfs
LSPWorkspace->>BamlVFS: resolve workspace root from platform path
BamlVFS->>NativePathBuf: convert VFS root to native path
NativePathBuf-->>BamlVFS: validated native root
LSPWorkspace->>BamlDiscovery: discover BAML files
BamlDiscovery-->>LSPWorkspace: sorted discovered paths
LSPWorkspace->>NativeVfs: read workspace files
NativeVfs-->>LSPWorkspace: file contents
sequenceDiagram
participant MonacoEditor
participant WorkspacePathModel
participant MonacoUri
participant FileSystem
MonacoEditor->>WorkspacePathModel: normalize filename
WorkspacePathModel->>MonacoUri: create contained file URI
MonacoUri-->>MonacoEditor: workspace file URI
MonacoEditor->>FileSystem: write and watch workspace files
FileSystem-->>MonacoEditor: URI-based file events
MonacoEditor->>WorkspacePathModel: extract relative filename
WorkspacePathModel-->>MonacoEditor: normalized relative filename
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/baml_workspace/src/discovery.rs`:
- Around line 18-19: Update the WalkBuilder configuration in discovery to call
require_git(false) alongside standard_filters(true), so .gitignore rules are
applied even when the root is not inside a Git repository, including the
.jj-only repository case.
In `@baml_language/crates/utils_windows/src/lib.rs`:
- Around line 62-68: Update native_path_from_vfs_path on the non-Windows path to
reject "." and ".." traversal components before constructing the PathBuf,
matching the Windows validation behavior while preserving existing invalid-path
handling.
In `@typescript2/pkg-editor/src/MonacoEditor.tsx`:
- Around line 668-669: Update the firstFile initialization in MonacoEditor so an
empty fileNames array produces a safe string fallback before calling
workspacePaths.fileUri. Preserve the existing firstFileIndex selection for
available text files, and ensure the fallback satisfies normalizeFilename’s
string requirement.
🪄 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: ec4345a1-84a2-4c0d-a3c8-442f2296add3
⛔ Files ignored due to path filters (2)
baml_language/Cargo.lockis excluded by!**/*.locktypescript2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (24)
baml_language/Cargo.tomlbaml_language/crates/baml_cli/src/format.rsbaml_language/crates/baml_cli/src/project_load.rsbaml_language/crates/baml_lsp_server/Cargo.tomlbaml_language/crates/baml_lsp_server/src/native_vfs.rsbaml_language/crates/baml_tests/benches/compiler_benchmark.rsbaml_language/crates/baml_tests/tests/emit_determinism.rsbaml_language/crates/baml_tests/tests/link_units_oracle.rsbaml_language/crates/baml_workspace/Cargo.tomlbaml_language/crates/baml_workspace/src/discovery.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/request.rsbaml_language/crates/bex_project/src/fs.rsbaml_language/crates/tools_compile_profile/src/main.rsbaml_language/crates/tools_onionskin/src/compiler.rsbaml_language/crates/utils_windows/Cargo.tomlbaml_language/crates/utils_windows/src/lib.rsbaml_language/sdk_tests/harness_setup/src/lib.rstypescript2/pkg-editor/package.jsontypescript2/pkg-editor/src/MonacoEditor.tsxtypescript2/pkg-editor/src/workspace-path.tstypescript2/pkg-editor/tests/workspace-path.test.tstypescript2/pkg-editor/vitest.config.ts
jubjub727
force-pushed
the
codex/fix-windows-portability
branch
from
July 21, 2026 12:21
86894e2 to
b85aa3c
Compare
Contributor
Author
|
Addressed the good code rabbit suggestions and the 1 hallucinated suggestion. Patching together the commits nicely here so there's only 1 commit per bug fix making it easy to reference later on. |
jubjub727
force-pushed
the
codex/fix-windows-portability
branch
from
July 21, 2026 12:24
b85aa3c to
c3b0512
Compare
sxlijin
reviewed
Jul 21, 2026
| } | ||
| } | ||
| let mut builder = WalkBuilder::new(root); | ||
| builder.standard_filters(true).follow_links(false); |
Contributor
There was a problem hiding this comment.
we want to follow symlinks
| if path.is_file() && path.extension().and_then(|s| s.to_str()) == Some("baml") { | ||
| files.push(path.to_path_buf()); | ||
| for entry in builder.build() { | ||
| let entry = entry?; |
Contributor
There was a problem hiding this comment.
we should do our best to make progress in a scenario like this, the filter_map(Result::ok) before was deliberate
| /// [`std::path::Path`]. Component casing is preserved; filesystem identity is | ||
| /// a separate policy owned by the project registry. | ||
| #[derive(Debug, Clone, PartialEq, Eq)] | ||
| pub struct NativePathBuf(PathBuf); |
Contributor
There was a problem hiding this comment.
what specifically does NativePathBuf as a newtype preclude, i.e. what states are unrepresentable because of NativePathBuf?
i'm wondering if having this as a trait makes more sense, since we end up using std::path::Path and PathBuf on both sides anyways
(i think there's two different ways to impl NativePathBuf that more obviously add type safety: (1) have it be NativePath::UnixPath | NativePath::WindowsPath, no cfg-only - this matters in e.g. the wasm case, where we run wasm on both windows and unix or (2) wrap VFS, ban direct access to it, and only allow using VFS through NativePathBuf)
Contributor
Author
There was a problem hiding this comment.
The actual underlying behaviour here seems to be a bit sticky. It's hard to get a clean solution that also addresses the nuance needed to make Windows work properly. I had Codex have a crack at it and it doesn't look terrible. Its solution is basically just ignoring native paths completely for wasm and ingesting them as strings on that platform which matches what wasm wants here.
I do agree that type safety is important here so I think it is worth getting a proper solution here instead of just accepting an easy solution.
I'm also moving away from the cfg heavy implementation here because it's a bit too fragile and makes the code harder to read imo. Going to split the windows behaviour and non windows behaviour into their own files so it's clearer what's going on here at a glance.
| Self { | ||
| root: self.root.clone(), | ||
| } | ||
| fn path_error(error: NativePathError) -> vfs::VfsError { |
Contributor
There was a problem hiding this comment.
should be possible to impl From<NativePathError> for VfsError
| @@ -0,0 +1,24 @@ | |||
| [package] | |||
| name = "utils_windows" | |||
Contributor
There was a problem hiding this comment.
might want to call this something else e.g. utils_fs if we're going to use it on all platforms
Contributor
Author
There was a problem hiding this comment.
Yep agreed using the utils_fs name now
Contributor
Author
|
I've implemented various changes and refactored things to minimize cfg spam and make things a bit more readable. |
Follow up on the Windows portability changes so cargo fmt --check passes. This only groups and orders imports; it does not change behavior.
sxlijin
reviewed
Jul 22, 2026
sxlijin
approved these changes
Jul 23, 2026
jubjub727
force-pushed
the
codex/fix-windows-portability
branch
from
July 24, 2026 13:46
46a89d1 to
5606379
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
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 `@typescript2/pkg-editor/src/MonacoEditor.tsx`:
- Around line 345-347: Update assertNotRoot in MonacoEditor.tsx to compare URI
identity using scheme, authority, and path rather than the full serialized URI,
so query or fragment decorations cannot bypass root protection. Preserve the
existing rejection behavior and add regression coverage for both delete and
rename operations using a decorated workspace root URI.
🪄 Autofix
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 Plus
Run ID: b5dfa1ae-a0b2-4033-bebc-43c0bfb6e197
⛔ Files ignored due to path filters (2)
baml_language/Cargo.lockis excluded by!**/*.locktypescript2/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
baml_language/Cargo.tomlbaml_language/crates/baml_lsp_server/Cargo.tomlbaml_language/crates/baml_lsp_server/src/native_vfs.rsbaml_language/crates/baml_path/Cargo.tomlbaml_language/crates/baml_path/src/lib.rsbaml_language/crates/baml_path/src/native.rsbaml_language/crates/baml_path/src/native/non_windows.rsbaml_language/crates/baml_path/src/native/windows.rsbaml_language/crates/baml_workspace/Cargo.tomlbaml_language/crates/baml_workspace/src/discovery.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/request.rsbaml_language/crates/bex_project/src/fs.rstypescript2/biome.jsontypescript2/pkg-editor/package.jsontypescript2/pkg-editor/src/MonacoEditor.tsxtypescript2/pkg-editor/src/workspace-path.tstypescript2/pkg-editor/tests/workspace-path.test.tstypescript2/pkg-editor/vitest.config.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- typescript2/pkg-editor/package.json
- baml_language/crates/baml_workspace/Cargo.toml
- typescript2/pkg-editor/vitest.config.ts
- baml_language/crates/bex_project/src/bex_lsp/multi_project/request.rs
- typescript2/pkg-editor/tests/workspace-path.test.ts
- typescript2/pkg-editor/src/workspace-path.ts
- baml_language/crates/baml_workspace/src/discovery.rs
- baml_language/crates/baml_lsp_server/src/native_vfs.rs
- baml_language/crates/bex_project/src/fs.rs
- baml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rs
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Merged
via the queue into
BoundaryML:canary
with commit Aug 14, 2026
cafaf3b
143 of 147 checks passed
meefs
pushed a commit
to meefs/baml
that referenced
this pull request
Aug 15, 2026
## Summary - add comprehensive release notes for the independent `baml_language` 0.17.0 release line - separate user-facing features, breaking and compatibility changes, fixes, performance work, and internal changes - synchronize the canary version, SDK packages, bridge runtime identities, generated Node loader guards, and VS Code extension to `0.17.0` ## Verified comparison range - previous release: `baml-language-0.16.0` at `2e021b3429db8769656e9b8646657c1863789169` - reviewed product target: `baml-language-source-c0153b20108bc428b77a53128b9a59ddeb4e2b42` at `c0153b20108bc428b77a53128b9a59ddeb4e2b42` - the reviewed product target was also `origin/canary` when the updated notes were prepared - full product comparison: BoundaryML/baml@baml-language-0.16.0...baml-language-source-c0153b20108bc428b77a53128b9a59ddeb4e2b42 The product range contains 43 monorepo commits. The notes cover every merged PR in the range that touched `baml_language/`, plus the baml_language-specific macOS test suppression and binary-size baseline workflow update. The post-BoundaryML#4424 review added BoundaryML#4427, BoundaryML#4431, BoundaryML#4432, BoundaryML#4433, BoundaryML#4434, BoundaryML#4103, and BoundaryML#4435. BoundaryML#3975 and BoundaryML#3510 are in the monorepo comparison but are excluded because they change only the main engine/docs release line. Other unrelated documentation, website, JetBrains, on-call, and general monorepo changes are also excluded. The five commits in this PR add and refine the changelog and mechanically prepare release metadata; they do not add product behavior beyond the reviewed target. ## Release safety This PR remains draft. Its `baml_language/release.toml` change is the documented canary release request: merging it to `canary` after green CI would dispatch the production 0.17.0 release workflow. Do not merge or mark ready until Sam explicitly approves the release. No package was published, no release tag was created or pushed, no artifact was uploaded, and no production release workflow was triggered during preparation. ## Validation - `scripts/baml-language-version sync` - `scripts/baml-language-version check` - `scripts/baml-language-version show` → `0.17.0` - `scripts/baml-language-version compute --channel canary` → `0.17.0` - `python3 -m unittest scripts.tests.test_baml_language_version scripts.tests.test_release_pipeline_contract` — 39 passed - `cargo test --manifest-path baml_language/Cargo.toml -p baml_version` - `mise run validate-markdown` from `baml_language/` - changed-file `prek` hooks, including Cargo formatting, Clippy, C++ formatting, TOML checks, and Markdown validation - `git diff --check` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added system and filesystem capabilities, cryptography utilities, time arithmetic, assertions, bitwise operations, improved indexing, toolchain pinning, and LSP code actions. - Added stable merge sorting for array helpers. - **Bug Fixes** - Improved language behavior, diagnostics, CLI evaluation, playground workflows, and language-server synchronization. - **Breaking Changes** - Updated string APIs, function field requirements, prompt syntax, and type-checking behavior. - **Documentation** - Added release notes for version 0.17.0. - **Release** - Published version 0.17.0 across supported SDKs and tooling. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Author Comment
Went through and made the first 3 Windows fixes. Codex found what imo looks like a really elegant solution for replacing
walkdir. Happy to make any requested changes. It looks good to me :)Codex Summary
This PR addresses the first batch of findings from a Windows path and URI audit across CLI discovery, the native LSP/VFS boundary, and the browser editor.
BAML file discovery previously used
walkdir, skipped excluded directories only after they had already been yielded, followed directory links, and silently discarded traversal errors. It now usesignore::WalkBuilderto prune ignored and hidden directories before descending, respect standard ignore files and negation rules, avoid following directory links, and return traversal failures to callers with context.Native filesystem paths and internal VFS paths were also being treated as interchangeable strings. This introduces a shared
utils_windowscrate containing a type-safe native path wrapper and the conversion contract between native paths and slash-oriented VFS paths. The conversion preserves Windows drive and UNC roots and is consumed directly by bothbex_projectandbaml_lsp_server.The remote Monaco editor previously created a file URI from the server workspace root but continued comparing and joining files using the original native path string. The editor now converts the workspace root to a URI once and derives file URIs, sandbox containment, watcher paths, ancestor directories, and relative filenames from that representation. Workspace-relative file keys are normalized and absolute or escaping paths are rejected.
Path casing is deliberately preserved. Filesystem identity and case-only path aliases are a separate concern and are not addressed by this batch.
Regression coverage verifies:
%, separator variants, containment boundaries, traversal attempts, and URI authority.Tested with the relevant Rust unit and integration suites, WASM checks, TypeScript tests and typechecks, and a production webview build.
Summary by CodeRabbit
New Features
Bug Fixes
Tests