Repository navigation
Conversation
Both node:url exports were bound directly to the Bun.fileURLToPath and Bun.pathToFileURL natives, which pick POSIX or Windows path semantics at compile time and take no options argument. Node >= 22.1 documents options.windows on both functions to force the other platform's semantics, which cross-platform tooling relies on to handle Windows file URLs on POSIX CI and vice versa. Wrap the natives so the host-platform fast path is unchanged, and port Node's getPathFromURLWin32/getPathFromURLPosix and the UNC-aware pathToFileURL encoder for the override case.
|
Warning Review limit reached
Next review available in: 8 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: ready for review. Tests pass locally on Linux x64 and Windows x64 (fail-before verified on both). Review feedback on forward-slash UNC handling addressed in 9f6e1a1. CI build 78615: 194 jobs passed, 0 failed. The remaining 2 jobs are Related: #33373 also adds |
|
Updated 7:29 AM PT - Jul 23rd, 2026
✅ @robobun, your commit 9f6e1a1277a0118adca3b17aa7cd973b60c09c35 passed in 🧪 To try this PR locally: bunx bun-pr 35260That installs a local version of the PR into your bun-35260 --bun |
|
Found 4 issues this PR may fix:
🤖 Generated with Claude Code |
|
On the bot's suggested issues: none of #28622, #14816, #29174 or #18748 are fixed here. Those all concern host-platform behaviour with no |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
path.win32.resolve normalizes forward-slash UNC roots (//srv/share) to \\srv\share, so the UNC branch must also fire when the resolved path starts with \\, not just when the raw input does. Matches Node's lib/internal/url.js.
There was a problem hiding this comment.
The forward-slash UNC issue from my earlier review is fixed in 9f6e1a1 and now matches Node's control flow; I didn't find further problems in the override path. Deferring to a human mainly because this overlaps with #33373 (native-side options.windows for fileURLToPath) and someone should decide which approach lands, and because ~160 lines of freshly ported lib/internal/url.js logic is more than I want to auto-approve.
What was reviewed:
getPathFromURLWin32/getPathFromURLPosixagainst Node's reference — drive-letter check, encoded-separator rejection, UNC hostname handling all match.pathToFileURLUNC branch now operates onresolved(extended-UNC prefix, empty-servername check at index 2, hostname assignment) — matches Node.encodePathCharstable covers the chars Node encodes explicitly; remaining C0/non-ASCII are handled bynew URL()'s path parser, so thefile://-prefixed-string approach is equivalent to Node'soutURL.pathname = ....- Host-platform fast path (
!windows === !isWindows) is unchanged, so no regression risk for callers that don't passoptions.
Extended reasoning...
Overview
Adds options.windows support to node:url's fileURLToPath and pathToFileURL by wrapping the existing Bun.fileURLToPath/Bun.pathToFileURL natives. When the option is unset or matches the host OS, behaviour is byte-identical to before (native fast path). When it overrides the host, ~160 lines of logic ported from Node's lib/internal/url.js (getPathFromURLWin32, getPathFromURLPosix, UNC-aware pathToFileURL with an encodePathChars helper) run instead. Two test files gain ~75 lines covering both override directions on both host platforms.
Security risks
None identified. This is pure path-string ↔ URL-string conversion with no filesystem, network, or privilege effects. Inputs are validated (validateString, isURL) before use; error paths use the centralized $ERR_* machinery. The urlPathEncodings lookup table uses __proto__: null.
Level of scrutiny
Medium. This is a Node-compat addition to a widely-used module (node:url), but the change is additive: existing callers that don't pass {windows} hit the unchanged native path, so regression surface is limited to the new option. The ported code is a close transcription of Node's reference and my earlier review already caught and fixed the one control-flow divergence (forward-slash UNC re-check after resolve). Tests cover drive letters, UNC, extended-UNC, percent-encoding validation, and round-tripping.
Other factors
- Overlap with #33373: that PR also implements
options.windowsforfileURLToPath, but on the C++ side. The author has acknowledged this in the thread. A maintainer should decide whether to land the JS-wrapper approach here (which also coverspathToFileURL), the native approach in #33373, or reconcile the two. This is the main reason I'm not auto-approving. - Prior review feedback addressed: my earlier finding on forward-slash UNC handling was fixed in 9f6e1a1 with a regression test added.
- Test gating: some
{windows: true}UNC assertions are guardedif (!isWindows)because on a Windows host they route through the nativeBun.pathToFileURL, whose extended-UNC handling is a pre-existing gap already tracked bytest.todo("UNC paths")in the same file. The comment explains this honestly rather than silently skipping.
|
Closing: the Verified against an unmodified main (b7a0431) debug build on Linux x64:
|
Problem
Node.js >= 22.1 documents an
options.windowsflag on bothurl.fileURLToPath(url, options)andurl.pathToFileURL(path, options)that forces Windows or POSIX path semantics regardless of the host OS. Cross-platform tooling relies on this to handle Windows file URLs on Linux CI and vice versa. Bun silently ignored the option and always applied host-OS semantics.Cause
src/js/node/url.tsexportedBun.fileURLToPathandBun.pathToFileURLdirectly. Those natives select POSIX or Windows behaviour via#if OS(WINDOWS)at compile time and accept no options argument, so the second argument was dropped on the floor. (Notably the siblingfileURLToPathBuffer, implemented in JS, already handlesoptions.windowscorrectly.)Fix
Wrap both exports in JS. When
options.windowsis unset or matches the host platform, fall through to the native (no behaviour or performance change for the common case). When it overrides the host, apply Node'sgetPathFromURLWin32/getPathFromURLPosixand the UNC-awarepathToFileURLencoder ported fromlib/internal/url.js.Verification
New tests in
test/js/node/url/url-fileurltopath.test.jsandtest/js/node/url/url-pathtofileurl.test.jsexercise{windows: true}and{windows: false}on both Linux and Windows, including drive-letter paths, UNC paths, percent-encoding validation and round-tripping. Both fail againstUSE_SYSTEM_BUN=1and pass with this change on Linux x64 and Windows x64.no test proof · iteration 1 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/url/url-fileurltopath.test.js test/js/node/url/url-pathtofileurl.test.js