resolver: keep forward slashes when imports target is a package specifier#30845
Conversation
…cifier Per the Node.js packages spec, values in imports/exports are URL-like specifiers and must always use forward slashes. When an imports entry maps a private name to a bare package specifier (e.g. "#res": "@myproject/resolver"), the resolver joined the target with the remaining subpath via platform::Auto and fed the result back into package-resolve. On Windows, platform::Auto is Windows, so the join normalized the / in the scoped-package name to \, producing @myproject\resolver. That no longer matches a scoped package on subsequent lookup and Bun falls back to the legacy main field instead of using exports. Use platform::Posix for this single join so the scoped-package / is preserved. The other platform::Auto joins in the same function operate on [package_url, str] / [package_url, str, subpath] where the result is a filesystem path and Windows normalization is correct. The Zig port (package_json.zig:1782) had the same .auto bug — matched here just in case the Zig is revived. Fixes #30839
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (12)
WalkthroughThis PR fixes scoped package imports resolution on Windows by switching path joining from platform-aware to Posix semantics, preserving forward slashes required for URL-like package specifiers. Supporting formatting refactors standardize multi-line platform configuration attributes across crash handling, runtime startup, process spawning, and system monitoring modules. ChangesWindows scoped package imports fix and platform configuration cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 2:57 PM PT - May 15th, 2026
❌ @robobun, your commit cdd4869 has 1 failures in 🧪 To try this PR locally: bunx bun-pr 30845That installs a local version of the PR into your bun-30845 --bun |
There was a problem hiding this comment.
LGTM — one-word resolver fix (Auto → Posix) with a targeted Windows regression test; the other 10 files are pure cargo fmt output from autofix.ci with no semantic change.
Extended reasoning...
Overview
The substantive change is a single identifier in src/resolver/package_json.rs (resolve_target): the path join for an imports target that is itself a bare package specifier now uses platform::Posix instead of platform::Auto. A Windows-only regression test is added to test/js/bun/resolve/resolve.test.ts. The remaining 10 files (crash_handler, errno, perf/tracy, runtime/cli/*, jsc_hooks, webview/ChromeProcess, spawn/*) are exclusively whitespace — #[cfg(...)] / #[cfg_attr(...)] attributes reflowed from single-line to multi-line form by the autofix.ci cargo fmt --all commit (8de894f), which follows the recently-merged #30682 that switched the format workflow to cargo fmt. I checked each hunk; none alter the predicate set or any token other than whitespace.
Security risks
None. The change narrows path normalization for a URL-like package specifier (per the Node.js packages spec, imports/exports values are always forward-slash). It does not touch filesystem-path joins (the two adjacent platform::Auto joins on package_url are correctly left as-is), auth, crypto, permissions, or any user-input parsing surface.
Level of scrutiny
Low. The logic delta is one word with a clear, well-argued PORT NOTE comment and PR description tying it to the Node spec and the original Zig line. On non-Windows targets platform::Auto already equals platform::Posix, so the change is a no-op everywhere except the broken Windows path it fixes. The new test mirrors the reporter's setup exactly and uses the established describe.if(isWindows) pattern. The autofix-applied formatting churn is mechanical and verifiably semantics-preserving.
Other factors
The robobun build failures cited are for commit b1d7b4d (pre-autofix) on the build-rust step across several targets; given the autofix commit that followed is purely cargo fmt output, those were format-check failures now resolved by 8de894f. No human reviewer comments are outstanding, no CODEOWNERS gating, and the bug-hunting system found nothing.
x bun-30845 test
bun test v1.3.14-canary.1 (8de894fec)
packages\main\bun\index.test.ts:
✓ Resolve 'resolver' module directly [1.45ms]
✓ Resolve 'resolver' module from subpath import [0.09ms]
✓ Resolve 'exporter' module directly [0.71ms]
✓ Resolve 'exporter' module from subpath import [0.08ms]It works 👀 Test repo: https://github.com/craftingmod/bun-subpath-import-slash |
There was a problem hiding this comment.
LGTM — one-line Posix-join fix for #30839 plus a Windows regression test; the other 10 files are pure cargo fmt reflows from autofix.ci.
Extended reasoning...
Overview
The substantive change is a single token swap in src/resolver/package_json.rs (platform::Auto → platform::Posix) inside the resolve_target branch that handles an imports target which is itself a bare package specifier (gated by internal && !./ && !../ && !/, returning Status::PackageResolve). A Windows-only regression test is added to test/js/bun/resolve/resolve.test.ts. The remaining 10 files (crash_handler, errno, tracy, Arguments.rs, run_command.rs, upgrade_command.rs, jsc_hooks.rs, ChromeProcess.rs, spawn/process.rs, spawn_sys/spawn_process.rs) are byte-for-byte semantic no-ops: #[cfg(...)]/#[cfg_attr(...)] attributes reflowed from single-line to multi-line by the [autofix.ci] apply automated fixes commit, following the recent switch of the format workflow to cargo fmt --all.
Security risks
None. No input parsing, auth, crypto, or privilege boundaries are touched. The resolver change only affects which path-separator style is used when concatenating a bare specifier with a subpath before re-entering package resolution; it cannot introduce traversal because the result is fed back into the package resolver (not opened as a filesystem path), and the ../ / / prefixes are already excluded by the surrounding guards.
Level of scrutiny
Low. The fix is one line whose correctness follows directly from the Node.js packages spec (imports/exports targets are URL-like, always /). On Linux/macOS Auto == Posix, so behavior is identical there; on Windows it now preserves the scoped-package / instead of normalizing to \. The PR description's analysis matches the code, the sibling PATTERN arm already builds a forward-slash specifier via string replace (no platform join), and the issue reporter independently confirmed the fix works on Windows. The formatting hunks require no review beyond confirming they are whitespace-only, which they are.
Other factors
A targeted Windows regression test is included and follows existing repo patterns (describe.if(isWindows), tempDir, junction symlink for the workspace link). CodeRabbit found nothing actionable, the bug-hunter system found no bugs, and there are no outstanding human review comments.
|
CI diff is green: all Windows lanes pass (the target of this fix), all macOS lanes pass, all Rust/C++ build lanes pass. The overall This change is in Stopping here per the CI-flake rule (one re-roll max). Needs a maintainer to re-run the flaky lane or merge. |
…fier (oven-sh#30845) ## Repro Per the reporter on Windows (also reproduced on main): ```jsonc // packages/app/package.json { "imports": { "#res": "@myproject/resolver" } } // packages/resolver/package.json { "name": "@myproject/resolver", "main": "./index.cjs", "exports": { ".": "./index.mjs" } } ``` ```js import { type } from '#res'; // Node: 'esm (from exports)' // Bun (Windows): 'cjs (from main)' ``` Linux/macOS output the expected ESM value; Windows falls back to the legacy `main` field. ## Cause In `src/resolver/package_json.rs` `resolve_target`, the branch that handles an `imports` target which is itself a bare package specifier (not `./…`, `../…`, `/…`) joins `[target, subpath]` via `resolve_path::platform::Auto` and hands the joined string back to package-resolve for a second pass. `platform::Auto` is `platform::Windows` on Windows, so the join calls `normalize_string_node_t::<u8, Windows>` which rewrites `/` to `\`. The scoped package name `@myproject/resolver` becomes `@myproject\resolver`; that no longer matches anything in `node_modules` and the resolver falls back to legacy main-field lookup, producing the CJS file. Per the Node.js packages spec, values inside `imports` and `exports` are URL-like specifiers that always use forward slashes — they are not OS-specific filesystem paths and must not be normalized to backslashes. The Zig reference (`package_json.zig:1782`) uses `.auto` here too; the Rust port inherited the bug. ## Fix Use `platform::Posix` for this one join so the scoped-package `/` is preserved. The other two `platform::Auto` joins in the same function operate on `[package_url, str]` / `[package_url, str, subpath]`, where the result IS a real filesystem path — Windows normalization there is correct and stays as-is. ## Test Added `describe.if(isWindows)("oven-sh#30839 - imports entry pointing at a scoped package", …)` in `test/js/bun/resolve/resolve.test.ts` that mirrors the reporter's setup: a workspace with `@myproject/resolver` dual-built (main=cjs, exports=mjs) and an `imports` entry mapping `#res` to the scoped name. Asserts that running `bun test.mjs` outputs the ESM value. The test is Windows-only because on Linux/macOS `platform::Auto` is already `platform::Posix` and this join is a no-op for forward-slash input; the misbehavior only surfaces when the join's platform is `Windows`. That matches the existing `test.if(isWindows)` regression pattern in the repo (e.g. `test/regression/issue/23292.test.ts`). ## Verification - Linux: `bun bd test test/js/bun/resolve/resolve.test.ts` — 37 pass, 2 skip (including the new Windows-only case), 0 fail. - Reproduction script from the issue, run against `build/debug/bun-debug` on Linux, prints `Resolved type: esm (from exports)` both before and after the fix (expected — Linux isn't affected). Fixes oven-sh#30839 --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Repro
Per the reporter on Windows (also reproduced on main):
Linux/macOS output the expected ESM value; Windows falls back to the legacy
mainfield.Cause
In
src/resolver/package_json.rsresolve_target, the branch that handles animportstarget which is itself a bare package specifier (not./…,../…,/…) joins[target, subpath]viaresolve_path::platform::Autoand hands the joined string back to package-resolve for a second pass.platform::Autoisplatform::Windowson Windows, so the join callsnormalize_string_node_t::<u8, Windows>which rewrites/to\. The scoped package name@myproject/resolverbecomes@myproject\resolver; that no longer matches anything innode_modulesand the resolver falls back to legacy main-field lookup, producing the CJS file.Per the Node.js packages spec, values inside
importsandexportsare URL-like specifiers that always use forward slashes — they are not OS-specific filesystem paths and must not be normalized to backslashes. The Zig reference (package_json.zig:1782) uses.autohere too; the Rust port inherited the bug.Fix
Use
platform::Posixfor this one join so the scoped-package/is preserved. The other twoplatform::Autojoins in the same function operate on[package_url, str]/[package_url, str, subpath], where the result IS a real filesystem path — Windows normalization there is correct and stays as-is.Test
Added
describe.if(isWindows)("#30839 - imports entry pointing at a scoped package", …)intest/js/bun/resolve/resolve.test.tsthat mirrors the reporter's setup: a workspace with@myproject/resolverdual-built (main=cjs, exports=mjs) and animportsentry mapping#resto the scoped name. Asserts that runningbun test.mjsoutputs the ESM value.The test is Windows-only because on Linux/macOS
platform::Autois alreadyplatform::Posixand this join is a no-op for forward-slash input; the misbehavior only surfaces when the join's platform isWindows. That matches the existingtest.if(isWindows)regression pattern in the repo (e.g.test/regression/issue/23292.test.ts).Verification
bun bd test test/js/bun/resolve/resolve.test.ts— 37 pass, 2 skip (including the new Windows-only case), 0 fail.build/debug/bun-debugon Linux, printsResolved type: esm (from exports)both before and after the fix (expected — Linux isn't affected).Fixes #30839