Skip to content

bundler: resolve relative Bun.build files keys against the cwd - #38650

Open
robobun wants to merge 7 commits into
mainfrom
farm/9bdf6f2a/files-relative-keys
Open

bundler: resolve relative Bun.build files keys against the cwd#38650
robobun wants to merge 7 commits into
mainfrom
farm/9bdf6f2a/files-relative-keys

Conversation

@robobun

@robobun robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • Bun.build({ files }) stores every key verbatim; the only normalization is \ to / (src/runtime/api/JSBundler.rs:105 on main). A relative key is therefore never equal to anything the bundler looks up by absolute path.
  • The two examples in the files docs and JSDoc are no-ops: with entrypoints: ["./src/index.ts"] on disk, files: { "./src/config.ts": ... } still bundles the disk copy, and files: { "./src/generated.ts": ... } fails with Could not resolve: "./generated.ts". Import lookups join the specifier onto the importer's directory (FileMap::resolve, bundle_v2.rs:1007 on main) and compare that absolute path with the relative key.
  • A relative key used as an entry point matches by byte equality and becomes a file namespace Path whose text is the relative key (result_for_key, bundle_v2.rs:1033 on main). Debug builds then die in enqueue_entry_item (bundle_v2.rs:2702) with panic: assertion failed: crate::is_absolute(self.text); an entry point with imports also reaches debug_assert!(bun_paths::is_absolute(source_dir)) in resolver.rs:1782. Release builds skip the assertions: a single-file relative entry point builds there, while one that imports another relative key fails to resolve it for the reason above.
  • Same function, same missing bound: the join uses the unchecked join_abs_string_buf, so import "./<5000 chars>.js" in any build that sets files aborts with panic: range end index 5003 out of range for slice of length 4095 (without files the same build reports a resolve error). A key of that length also crashes, in the pretty-path computation downstream.
  • On Windows the store side uppercases the drive letter and the lookup side (path_to_posix_buf) does not, so a c:/... key never matches the entry point it was written for.

Fix

  • FileMap (src/bundler/bundle_v2.rs) gets one canonical spelling for a path, FileMap::canonical: / separators (uppercase drive letter on Windows, as the store side already did), and a path that is not absolute is resolved against FileSystem::top_level_dir, the directory relative entry points are resolved against. Absolute keys keep their text, so nothing changes for the absolute keys every existing test uses.
  • FileMap::put stores keys in that spelling and get / contains / resolve canonicalize their input before probing the map, so a key and a path can only ever meet in one spelling. For an import, resolve still joins the specifier onto the importer's directory and then canonicalizes the joined path; for an entry point (empty source_file) or an absolute specifier it canonicalizes the specifier itself. The byte-equality probe of the raw specifier is gone: a relative key is the file it resolves to, not a pattern that matches that specifier text from every importer (the docs and JSDoc describe keys as paths; the new test "a key only matches the file it resolves to" pins this down).
  • This is why the fix is right: the assertions are the invariant "a file namespace path is absolute", and user input can only satisfy it if it is resolved where it enters, in put, which is also exactly what makes the documented override and virtual-file examples work, because import lookups were already comparing absolute paths. bundler: allow relative FileMap keys without tripping absolute-path debug asserts #32716 instead relaxes the two debug assertions and leaves the documented examples broken; it is closed in favour of this PR. Its four cases (three spellings of a relative entry point and the parse-error input) pass against a debug build of this branch, and the parse-error one is in the test file below.
  • lookup_import puts the importer through the same bounds-checked canonical (it used to copy it into two path buffers unchecked, so a plugin module whose path is longer than a buffer crashed any build that sets files once it imported a relative specifier: panic: range end index 100012 out of range for slice of length 4095), takes bun_paths::dirname, and joins the specifier with join_abs_string_buf_checked, probing the map with the joined path directly instead of canonicalizing it a second time (two pool buffers at a time instead of five, with the pool holding four). An importer or specifier that does not fit is simply not in the map and the resolver reports it, as it does when files is not set. A key whose resolved path does not fit in a path buffer is rejected by file_map_from_js with TypeError: files: key resolves to a path longer than N bytes (the Bun.mmap precedent in BunObject.rs); before, such a key was accepted and crashed the build.
  • file_map_from_js (src/runtime/api/JSBundler.rs) now just hands the key to put; the separator conversion moved into canonical. The two FileSystem::instance() reads in the moved code became FileSystem::get(), the shared accessor that type documents for read-only use.
  • JSDoc and docs/bundler/index.mdx state the rule (relative keys resolve against the cwd like relative entrypoints); the examples there were already written that way.
  • Verified with test/bundler/bundler_files.test.ts (new relative keys block, 19 tests: the two documented examples in four key spellings including .\ and .. segments, relative keys overriding the files the disk resolver picks for an extensionless and a package import (the FileMap::get route, which no other test reaches with a relative key), an entry point matrix mixing spellings of the entry point and its keys with the files importing each other, an in-memory entry importing a disk file, a syntax error in a relatively keyed entry point, the path-not-pattern case, an over-long key, specifier and importer path, and a Windows-only drive-letter case):
    • bun bd test test/bundler/bundler_files.test.ts: 41 pass, 1 skipped (Windows-only).
    • bun bd test with src/ stashed (debug, unfixed), on the first 16 tests: all 15 non-Windows tests fail, 6 of them on assertion failed: crate::is_absolute(self.text); the 23 existing tests pass.
    • USE_SYSTEM_BUN=1 bun test, same 15: 13 fail (both over-long tests exit 134); the two that release passes by accident (identical spelling everywhere, in-memory entry importing a disk file) are the ones that only fail on the assertions above.
    • The three tests added afterwards: the syntax error input panics an unfixed debug build on the entry point assertion, and release reports the key's relative text as the error's file; the resolver-override test bundles the disk copies on release; the over-long importer test crashes a debug build of this branch as it stood before the lookup_import change (exit 134, the panic above), and on release the removed byte-equality probe makes it success: true. All three fail there and pass here.
    • bun bd test test/js/bun/css/doesnt_crash.test.ts (the other user of files, absolute keys): 61 pass, re-run on the final tree.
    • Windows x64: USE_SYSTEM_BUN=1 (1.4.0) fails the same 13 plus the drive-letter test (ModuleNotFound resolving "c:\...\entry.js" (entry point)); bun bd test of this branch (first 16 tests): 39 pass; the three later tests have only run on Linux locally and rely on CI for Windows.
  • main was merged in to resolve a conflict with the docs voice pass (docs: voice pass over docs/ #38760) in docs/bundler/index.mdx.
  • Two adjacent pre-existing bugs found while verifying are left alone and reported separately: a source path just under MAX_PATH_BYTES still overflows in generic_path_with_pretty_initialized, and the [dir] fallback in computeChunks.rs for an entry whose directory does not exist on disk drops the leading slash (so a virtual entry in a subdirectory gets a cwd-prefixed output path; this already affects absolute keys today and, with this change, relative ones the same way).

Background

  • files is the Bun.build option that maps paths to in-memory contents. FileMap is consulted in three places: when an entry point is enqueued (file_map.resolve(arena, b"", entry_point)), when an import record is resolved (resolve(arena, importer_path, specifier), before the disk resolver runs), and when a parse task reads a source (get(path), which is how in-memory contents replace a file that also exists on disk).
  • FileSystem::top_level_dir (bun_resolver) is the process cwd as the bundler sees it; process.chdir updates it, and resolve_entry_point resolves relative entry points against it, which is why keys are resolved against the same directory.
  • file namespace paths are assumed absolute throughout the bundler (Path::assert_file_path_is_absolute, source_dir in the resolver); the checks are debug_asserts, so release builds do not panic on a relative one, they just compute relative-to-nothing paths.
  • path_buffer_pool hands out MAX_PATH_BYTES buffers (4096 on Linux, 1024 on macOS, ~98 KB on Windows); the _checked join variants return None instead of writing past one, which is the bound the over-long specifier test trips on every platform with a 100k-character specifier.

no test proof · iteration 2 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_files.test.ts

The files map stored each key verbatim, so a relative key never matched
anything the bundler looked up by absolute path: the documented
"./src/config.ts" override and "./src/generated.ts" virtual file were
ignored, and a relative key used as an entry point produced a file
namespace Path with relative text, which trips the is_absolute
assertions in enqueue_entry_item and the resolver on debug builds.

FileMap now has one canonical spelling for keys (forward slashes,
relative paths resolved against the cwd like relative entry points)
that put() and every lookup go through, so keys are compared with entry
points, import targets and source paths in one spelling only. The
relative import join uses the checked variant, so an over-long import
specifier in a build with files set is a resolve error instead of a
buffer overflow panic, and a key whose resolved path does not fit in a
path buffer is rejected when the options are parsed.
@robobun
robobun requested a review from alii as a code owner August 14, 2026 19:12
@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c494b1ea-bd43-475e-96cb-66d0c5ff1753

📥 Commits

Reviewing files that changed from the base of the PR and between cfd3bea and 0ce36e3.

📒 Files selected for processing (5)
  • docs/bundler/index.mdx
  • packages/bun-types/bun.d.ts
  • src/bundler/bundle_v2.rs
  • src/runtime/api/JSBundler.rs
  • test/bundler/bundler_files.test.ts

Comment @coderabbitai help to get the list of available commands.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Status: fix and tests pushed, waiting for CI.

Reproduced on bun 1.4.0 and on a debug build of main: with cwd holding src/index.ts (imports ./config.ts, which exists on disk), Bun.build({ entrypoints: ["./src/index.ts"], files: { "./src/config.ts": ... } }) bundles the disk copy, and files: { "./src/generated.ts": ... } fails with Could not resolve: "./generated.ts"; a relative key used as an entry point panics a debug build with assertion failed: crate::is_absolute(self.text) in enqueue_entry_item. Keys were stored verbatim, so they were never equal to the absolute paths the bundler looks up.

Verification is in the PR description (test/bundler/bundler_files.test.ts, "relative keys" block: fails on the unfixed debug build and on 1.4.0, passes with the fix). #32716 addresses the assertion half of this by relaxing the asserts; this PR supersedes it.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. bundler: allow relative FileMap keys without tripping absolute-path debug asserts #32716 - Fixes the same bug (relative Bun.build({ files }) keys used as entry points) the opposite way, by relaxing the enqueue_entry_item / generate_isolated_hash debug assertions instead of canonicalizing keys in FileMap::put.

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it changes bundler FileMap resolution semantics (relative keys now resolve against the cwd; the byte-equality raw-specifier probe is removed) and touches CODEOWNER-protected packages/bun-types/bun.d.ts, a human look would still be worthwhile.

What was reviewed:

  • FileMap::canonical / put / lookup / lookup_import — separator + drive-letter normalization via dangerously_convert_path_to_posix_in_place, cwd join via join_abs_string_buf_checked, and that absolute keys keep their text so existing absolute-key tests are unaffected.
  • Over-long inputs — put returns MaxPathExceeded (surfaced as a TypeError in file_map_from_js), and lookup_import uses the _checked join so a long specifier falls through to the resolver instead of panicking.
  • Removal of the direct raw-specifier probe in resolve — pinned by the new "a key only matches the file it resolves to" test; consistent with the docs' "keys are file paths" wording.
Extended reasoning...

Overview

This PR makes Bun.build({ files }) treat relative keys as paths resolved against the process cwd (matching how relative entrypoints resolve), instead of storing them verbatim. It refactors FileMap in src/bundler/bundle_v2.rs around a single canonical spelling that both put and every lookup path (get/contains/resolve/lookup_import) use, so keys and lookups can only meet in one form. src/runtime/api/JSBundler.rs is simplified to hand raw keys to put and surface a TypeError when the resolved key exceeds MAX_PATH_BYTES. Docs (docs/bundler/index.mdx) and JSDoc (packages/bun-types/bun.d.ts) are updated to state the rule, and 16 new subprocess tests are added in test/bundler/bundler_files.test.ts.

Security risks

None identified. The change only affects how in-memory files keys are matched inside the bundler; no filesystem writes, no network, no auth/crypto surface. The over-long-path handling is a hardening (crash → typed error / resolve error).

Level of scrutiny

This is a real behavioral change to bundler resolution, not a mechanical fix. Two things in particular deserve human sign-off: (1) the design decision that a relative key is a cwd-relative path rather than a specifier pattern — the old resolve had a byte-equality probe of the raw specifier that this PR removes, which the PR argues (reasonably) never worked coherently and contradicted the docs; and (2) cross-platform path normalization on Windows (drive-letter casing, separator conversion, the Loose join platform), which I could only verify by reading dangerously_convert_path_to_posix_in_place and the Windows-only test rather than by execution.

Other factors

packages/bun-types/bun.d.ts is CODEOWNER-protected. The PR is very thoroughly tested (matrix of key/entrypoint spellings, over-long key and specifier, Windows drive-letter case, path-not-pattern semantics) and the description documents both bun bd test and USE_SYSTEM_BUN=1 runs plus the other files consumer (doesnt_crash.test.ts). It also supersedes #32716. Given the semantic change and CODEOWNER coverage, deferring rather than auto-approving.

@robobun

robobun commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Re the duplicate check: #32716 and this PR are alternatives for the same assertion failure, not the same change. #32716 relaxes the two debug assertions and keeps relative keys relative; this PR resolves the keys against the cwd when the options are parsed, so the assertions hold as written, and the documented relative-key override / virtual-file examples start working (they are no-ops today because import lookups compare absolute paths). Only one of the two should land; I left a note on #32716 pointing here.

Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:05 PM PT - Aug 15th, 2026

@robobun, your commit 0ce36e3 has some failures in Build #98365 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38650

That installs a local version of the PR into your bun-38650 executable, so you can run:

bun-38650 --bun

Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated
Comment thread src/bundler/bundle_v2.rs Outdated

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Deferring to a human reviewer because it changes the user-facing semantics of Bun.build({ files }) — relative keys now resolve against the cwd and the raw byte-equality specifier match is intentionally removed — and it competes with #32716 for the same fix; a maintainer should decide which approach lands.

What was reviewed:

  • FileMap::canonical / put / lookup / lookup_import in bundle_v2.rs — store and lookup both go through the same canonicalization; absolute keys are unchanged.
  • Windows drive-letter handling — dangerously_convert_path_to_posix_in_place uppercases the drive on both sides now, matching the new test.
  • Overflow paths — join_abs_string_buf_checked and the scratch.get_mut(..path.len())? bound turn overlong keys/specifiers into None / a thrown TypeError instead of a slice panic.
  • file_map_from_js — capacity is still reserved before put_assume_capacity; the key slice is copied into the map so the pooled buffer can be returned.
Extended reasoning...

Overview

The PR rewrites FileMap in src/bundler/bundle_v2.rs so that keys in Bun.build({ files }) are stored and looked up in one canonical spelling: forward slashes, uppercase drive letter on Windows, and — the substantive change — a non-absolute key is joined onto FileSystem::top_level_dir (the cwd). get/contains/resolve all funnel through the same canonical helper, and resolve drops the old raw-specifier byte-equality probe in favour of resolving imports against the importer's directory and then canonicalizing. file_map_from_js in JSBundler.rs now delegates key normalization to FileMap::put and throws a TypeError when the resolved key would exceed MAX_PATH_BYTES. Docs, JSDoc, and 16 new tests in bundler_files.test.ts accompany the change.

Security risks

None identified. The change tightens bounds handling (checked joins, length-guarded scratch buffer) rather than loosening any. Inputs are user-supplied build options, not remote data.

Level of scrutiny

High. This is a semantic change to a public Bun.build option that the docs already advertise, it removes a previously-observable behavior (a relative key matching any importer whose specifier text is byte-equal), and it rewrites ~150 lines of cross-platform path handling in the core bundler. It also directly competes with open PR #32716, which fixes the same debug assertion by relaxing the invariant instead of canonicalizing keys — the description explicitly says only one should land. That is a maintainer-level decision.

Other factors

The implementation looks sound and the test coverage is thorough (spelling matrix, disk/memory mixing, syntax-error path reporting, path-not-pattern semantics, both overlong-input cases, Windows drive-letter case). I confirmed dangerously_convert_path_to_posix_in_place uppercases the drive letter under cfg(windows), so store and lookup now agree there. The comment-cop bot's inline notes were addressed in 27fd93f and are all resolved. Given the API-design choice and the competing PR, this should be signed off by a human.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it reworks FileMap resolution semantics in the bundler (relative keys now resolve against the cwd, and the raw-specifier byte-equality probe is removed) and supersedes #32716, a human sign-off on the chosen semantics would be worthwhile.

What was reviewed:

  • FileMap::canonical / put / lookup / lookup_import — bounds-checked via get_mut(..len) and join_abs_string_buf_checked; store and lookup sides now share one normalization (dangerously_convert_path_to_posix_in_place, which uppercases the drive letter on Windows), fixing the c:/ mismatch.
  • Call sites (resolve at entry-point enqueue, two import-record sites, and get in ParseTask) — the empty-source_file entry-point path and the ParseTask get(path) path both go through lookup, so keys stored via put round-trip.
  • file_map_from_jsput failure surfaces as a TypeError before the build starts; prop.to_utf8() replaces the owned-slice clone.
  • Tests — the new relative keys block spawns a subprocess in the temp dir (via bunRun's cwd = dirname(file)), so the process-global top_level_dir is exercised without chdir-ing the runner.
Extended reasoning...

Overview

The PR canonicalizes Bun.build({ files }) keys: FileMap::put now resolves each key to an absolute, /-separated path (against FileSystem::top_level_dir, the same base relative entrypoints use) and every lookup (get/contains/resolve) canonicalizes its input the same way before probing. resolve is split into lookup (entry point / absolute specifier) and lookup_import (relative specifier joined onto the importer's directory). The unchecked join_abs_string_buf is replaced with the _checked variant, and over-long keys are rejected in file_map_from_js with a TypeError. Docs and JSDoc are updated, and 16 new tests cover the documented examples, mixed key/entrypoint spellings, the path-not-pattern rule, over-long inputs, and a Windows drive-letter case.

Security risks

None identified. The inputs are user-supplied build-option paths that are only used as hash-map keys and file-namespace Path text; no filesystem writes or shell involvement. The over-long-path handling closes a user-reachable panic (slice out of bounds), which is a robustness fix, not a security boundary.

Level of scrutiny

Moderate-to-high. FileMap sits in the bundler's resolve path (consulted before the disk resolver for every import when files is set) and the change embeds a semantic decision: relative keys are now file paths resolved against the cwd, and the old byte-equality match against the raw import specifier is intentionally removed (pinned by "a key only matches the file it resolves to"). That is the right reading of the docs and fixes the debug-build assertion, but it is a user-visible behaviour choice that also competes with #32716's alternative fix, so a maintainer should confirm which one lands.

Other factors

  • The refactor preserves the pre-existing lookup_import body (abs_buf / path_to_posix_buf / dirname / drive-root fallback) apart from swapping to join_abs_string_buf_checked and calling self.lookup instead of get_key_value — the diff is largely a move.
  • FileSystem::instance()FileSystem::get() is the shared-read accessor the type recommends; abs_buf and top_level_dir are read-only.
  • dangerously_convert_path_to_posix_in_place is confirmed to uppercase the drive letter on Windows and convert \\/ on every platform, so store and lookup now agree; the old path_to_posix_buf lookup did neither the drive-letter fold nor a bounds check.
  • Test coverage is thorough (fails on the unfixed build, passes with the fix, verified on Linux and Windows per the description), the existing absolute-key tests are untouched, and the other files consumer (doesnt_crash.test.ts) was re-run.
  • The comment-cop threads on the earlier revision were addressed in 27fd93f and are all resolved.

The importer was copied into two path buffers unchecked, so a plugin
module whose path is longer than a path buffer crashed any build that
sets `files` as soon as it imported a relative specifier. Canonicalize
the importer the same way keys are, which is bounds-checked, take its
dirname, and probe the map with the joined path directly instead of
canonicalizing it a second time.

Also cover relative keys overriding files the disk resolver picks
(extensionless and package imports), which only works through
FileMap::get.
Comment thread src/bundler/bundle_v2.rs Outdated
@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Folded #32716 into this PR and closed it; its cases pass here and the PR description has the details (including the importer bounds fix in 591350c).

Ready for review: all review threads are addressed. The last two CI runs (98207, 98365) fail only on unrelated flaky suites, each of which passed on retry or when rerun alone; the bundler files tests are green on every lane.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it changes the user-visible semantics of Bun.build({ files }) keys (relative keys now resolve against the cwd, and the raw-specifier byte-equality probe is removed) and supersedes a competing approach in #32716, a maintainer should sign off on the design choice.

What was reviewed:

  • FileMap::canonical / put / lookup / lookup_import — path-buffer bounds via join_abs_string_buf_checked and scratch.get_mut(..len), no unchecked writes remain.
  • Windows drive-letter canonicalization — dangerously_convert_path_to_posix_in_place uppercases the drive on both store and lookup sides.
  • Thread affinity of FileSystem::get().top_level_dir — documented shared-ref accessor already used from bundler threads.
  • Call sites of FileMap::get/contains/resolve in bundle_v2.rs and ParseTask.rs — all pass a path that canonical handles.
Extended reasoning...

Overview

The PR rewrites FileMap (src/bundler/bundle_v2.rs) so Bun.build({ files }) keys and lookups meet in one canonical spelling: / separators, uppercased drive letter on Windows, and non-absolute paths resolved against FileSystem::top_level_dir. put now canonicalizes and bounds-checks the key; get/contains/resolve canonicalize their input; lookup_import joins the specifier onto the importer's canonical directory with the checked join. file_map_from_js (src/runtime/api/JSBundler.rs) hands the raw key to put and throws TypeError on MaxPathExceeded. Docs, JSDoc, and 16 new subprocess tests in test/bundler/bundler_files.test.ts cover the relative-key spellings, the path-not-pattern rule, over-long keys/specifiers/importers, and a Windows drive-letter case.

Security risks

None identified. Input is user-provided build configuration; the only new failure mode (over-long resolved path) is now a synchronous TypeError instead of a later panic. No filesystem writes or privilege changes.

Level of scrutiny

High. This is a deliberate semantic change to a user-facing API: relative files keys now mean "the file this path resolves to from the cwd" instead of "the literal text of an import specifier". The old raw-specifier byte-equality probe is removed, so a relative key no longer matches every importer that spells the same specifier. The PR argues (convincingly) that the old behavior was never documented as such and that the docs/JSDoc already described keys as paths — but a maintainer should confirm they want the stricter interpretation and pick this over #32716's assertion-relaxing alternative. Cross-platform path handling (bun_paths::dirname, join_abs_string_buf_checked, drive-letter casing) is subtle enough to merit a human eye even though I traced each helper and found nothing wrong.

Other factors

  • Test coverage is thorough: matrix over key spellings, both docs examples, extensionless/package resolver hits, entry-point spelling matrix, syntax-error path attribution, three over-long-path bounds tests, and a Windows-only drive-letter test. The PR description records that all 15 non-Windows new tests fail on the unfixed debug build and 13 fail on release 1.4.0.
  • One comment-cop inline comment on bundle_v2.rs:973 remains unresolved; it flags the two-line doc on lookup_import, which reads as a bot false positive rather than a real paragraph-long justification.
  • No prior review from me on this PR.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and the automated bug hunt found no issues. Because it's a ~180-line refactor of FileMap's path canonicalization in the bundler — including a deliberate semantic change (dropping the raw-specifier byte-equality probe so relative keys are paths, not patterns), Windows drive-letter/separator handling, and a switch from resolve_path::dirname to the Option-returning bun_paths::dirname — a human look would still be worthwhile, particularly to sign off on the resolve-against-cwd design vs. #32716's approach.

What was reviewed:

  • canonical/put/lookup/lookup_import: bounds-checking via get_mut/join_abs_string_buf_checked, buffer-pool usage (two live buffers vs. pool of four), and that dangerously_convert_path_to_posix_in_place uppercases drive letters on Windows so store and lookup agree.
  • The removed drive-root/"/"/cwd fallback for dirname — the new bun_paths::dirname returns Some for root-level importers, and the 23 pre-existing absolute-key tests still exercise that path.
  • file_map_from_js: over-long keys now throw TypeError instead of crashing downstream; matches the Bun.mmap precedent.
Extended reasoning...

Overview

This PR reworks FileMap (the Bun.build({ files }) in-memory file map) in src/bundler/bundle_v2.rs so that keys and lookups share one canonical spelling: forward slashes, uppercase drive letter on Windows, and non-absolute paths resolved against FileSystem::top_level_dir (the cwd). put stores the canonical key; get/contains/resolve canonicalize before probing. resolve is split into lookup (entry point / absolute specifier) and lookup_import (relative specifier joined onto dirname(importer)), both bounds-checked. file_map_from_js in JSBundler.rs now delegates to put and throws on over-long keys. Docs/JSDoc are updated to state the rule, and 19 subprocess tests are added.

Security risks

None identified. This is path canonicalization for an in-process build API; no filesystem writes, no privilege boundaries. Over-long inputs that previously panicked now return None or throw a TypeError.

Level of scrutiny

High. This is resolver-adjacent logic with cross-platform path handling and a deliberate user-facing semantic change: the old code probed the map with the raw specifier text first (so files: { "./config.ts": ... } matched import "./config.ts" from any directory), and that probe is removed. The PR argues this is correct (keys are paths, per the docs) and pins it with a test, but it is still a design decision a maintainer should confirm — especially since it supersedes #32716, which took the opposite approach of relaxing the assertions.

Other factors

  • The old resolve had explicit fallback logic when resolve_path::dirname::<Posix> returned empty (drive root, "/", cwd); the new code uses the Option-returning bun_paths::dirname and short-circuits on None. The existing absolute-key tests (e.g. /entry.js importing ./utils.js) cover the root-level importer case and are reported passing, but the reviewer should confirm Windows drive-root behavior is still equivalent.
  • canonical reads FileSystem::get().top_level_dir on both the JS thread (put) and the bundler thread (lookup); the old resolve already read it on the bundler thread, so this is not a new cross-thread access, but worth noting.
  • Test coverage is thorough (variant matrix, over-long key/specifier/importer, Windows drive-letter, syntax-error path), verified against unfixed debug and release builds per the description; three later tests rely on CI for Windows.

@robobun

robobun commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator Author

This PR also fixes #39252: Bun.build({ files }) panics with "range end index N out of range for slice of length PATH_MAX-1" when an in-memory file imports a data: URL longer than PATH_MAX, because the same unchecked join in FileMap::resolve treats the URL as a relative path. The checked join here covers it (the specifier falls through to the real resolver, which parses data: URLs).

I had opened #39256 with a narrower version of the same fix before noticing this PR; closing it in favor of this one. Consider adding "Fixes #39252" to the description, and feel free to lift the regression test from my branch (farm/4d8d1627/filemap-data-url-pathmax, test/bundler/bundler_files.test.ts): it bundles a 100000-byte CSS data: URL from an in-memory file, sized to exceed the path buffer on every platform including Windows (98302 bytes).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant