bundler: emit posix-relative paths in the HTML-import manifest on Windows - #34557
bundler: emit posix-relative paths in the HTML-import manifest on Windows#34557robobun wants to merge 5 commits into
Conversation
|
Warning Review limit reached
Next review available in: 27 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 (6)
WalkthroughGenerated paths across bundler manifests, copied assets, standalone metadata, sourcemaps, and module maps now use POSIX separators and consistent leading ChangesPath normalization across generated outputs
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:11 AM PT - Jul 18th, 2026
❌ @robobun, your commit 08930b7 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34557That installs a local version of the PR into your bun-34557 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
LGTM — targeted Windows path fix that mirrors the existing final_rel_path / sourcemap-sources pattern.
What was reviewed:
- Verified
relative_platform_buf::<Auto, true>+platform_to_posix_in_placematches the pattern atgenerateChunksInParallel.rs:372andLinkerContext.rs:1087; both helpers early-return on POSIX so Linux/macOS output is unchanged. - Checked
path_buffer_pool::get()returns an RAIIPoolGuard, and that the per-iteration buffer reuse is sound (each borrow is consumed bywrite_entry_itembefore the next write). - Confirmed
ALWAYS_COPY = trueis required since the result is mutated in place;platform_to_posix_bufis used fordest_pathbecause it's a borrowed slice.
Extended reasoning...
Overview
Two-file change: src/bundler/HTMLImportManifest.rs swaps relative_normalized::<Posix, false> for relative_platform_buf::<Auto, true> + platform_to_posix_in_place when computing manifest input keys, and posix-converts output_file.dest_path before emitting it. A new test in test/bundler/html-import-manifest.test.ts asserts every manifest path is /-separated and project-relative.
Security risks
None. This is pure output-path formatting inside the bundler's manifest writer — no untrusted input parsing, no filesystem writes driven by the changed values, no auth/crypto surface.
Level of scrutiny
Low-to-medium. The change is mechanical: it replaces one path helper call with the established "relative under host semantics, then rewrite separators" idiom already used at generateChunksInParallel.rs:372 (chunk final_rel_path) and LinkerContext.rs:1087 (sourcemap sources). Both platform_to_posix_in_place and platform_to_posix_buf early-return when SEP == b'/', so POSIX output is byte-identical — confirmed by the two existing inline snapshots in the test file being untouched.
Other factors
- The pooled buffers come from
path_buffer_pool::get(), which returns aPoolGuardwithDrop-based return-to-pool, so no manual cleanup is needed on the?error paths. ALWAYS_COPY = trueis correct: the result must live ininput_bufsoplatform_to_posix_in_placecan rewrite it.- Buffer reuse across loop iterations is sound — each
&input_buf[..len]borrow is fully consumed bywrite_entry_itembefore the next iteration re-borrows mutably. - The new test uses
Bun.buildin-process (matching the neighbouringetag-changes-with-referenced-chunkstest) and asserts exactinputvalues plus a no-backslash sweep, so it fails on Windows without the fix and passes on POSIX both before and after. The PR description is upfront that the Linux fail-before gate will not catch this — Windows CI is the direct proof. - The PR correctly notes the remaining 8.3-short-path vs long-path mismatch as a separate, pre-existing issue and does not attempt to paper over it here.
Instead of re-normalising dest_path at a fourth consumer, apply the same platform_to_posix_in_place that Chunk.final_rel_path already gets after PathTemplate::print. Every reader either already re-normalised, routes through platform::Auto/Loose path helpers that accept '/', or wants a URL/module key where '/' is required; Windows openat/mkdirat normalise '/' before the NT syscall. Drops the now-redundant workarounds in Chunk.rs, HTMLImportManifest.rs, StandaloneModuleGraph.rs and bake/production.rs.
2179c7e to
2de0f0a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@test/bundler/html-import-manifest.test.ts`:
- Around line 387-398: Update the manifest assertions around byLoader so they
retain and validate every manifest entry instead of only the last file per
loader. For manifest.index, each file.path, and each defined file.input, assert
both posix.isAbsolute and win32.isAbsolute are false, while preserving the POSIX
separator checks and existing loader-specific expectations.
- Around line 367-373: Update the test fixture setup around tempDirWithFiles to
use the disposable tempDir helper with using, and update the harness import
accordingly. Preserve the existing fixture files and test behavior while
ensuring the temporary directory is automatically cleaned up on every execution
path.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e197cc68-1f62-4b9b-88dc-1e73ad8ea2bb
📒 Files selected for processing (6)
src/bundler/Chunk.rssrc/bundler/HTMLImportManifest.rssrc/bundler/bundle_v2.rssrc/runtime/bake/production.rssrc/standalone_graph/StandaloneModuleGraph.rstest/bundler/html-import-manifest.test.ts
💤 Files with no reviewable changes (2)
- src/standalone_graph/StandaloneModuleGraph.rs
- src/bundler/Chunk.rs
e083d4e to
08930b7
Compare
|
The Windows CI on this PR is green ( Remaining CI failures are unrelated to this diff: |
There was a problem hiding this comment.
I didn't find any issues. The producer-side normalization mirrors the existing Chunk.final_rel_path invariant and the removed consumer workarounds are demonstrably redundant, but this is a cross-cutting Windows path-format change spanning bundler → standalone graph → bake, so it's worth a maintainer glance once Windows CI is green.
What was reviewed:
- Confirmed
platform_to_posix_in_placeearly-returns on POSIX (resolve_path.rs:2458), so Linux/macOS output is unchanged. - Confirmed
Chunk.final_rel_pathalready carries the same posix invariant (generateChunksInParallel.rs:372) through the same disk-write /write_to_diskconsumers, so extending it toadditional_output_files[].dest_pathintroduces no new consumer requirement. - Checked
writeOutputFilesToDisk.rsandOutputFile::write_to_diskroutedest_paththroughplatform::Autodirname +bun_sysopenat, which accept/on Windows. bun_paths::strings::remove_leading_dot_slashalready handles both./and.\\, so thebake/production.rsinline checks were exact duplicates of the shared helper.
Extended reasoning...
Overview
Two-part Windows path-format fix in the bundler. (1) HTMLImportManifest::write switches the manifest input derivation from relative_normalized::<Posix, false> on native absolute paths (which mis-parses C:\\... as one segment) to relative_platform_buf::<Auto, true> + platform_to_posix_in_place, using a pooled buffer. (2) bundle_v2.rs posix-converts additional_output_files[].dest_path immediately after PathTemplate::print, matching the existing normalization applied to Chunk.final_rel_path at the same stage. Three consumer-side workarounds that had each independently re-normalized this field (Chunk.rs scratch-buffer copy, StandaloneModuleGraph.rs #[cfg(windows)] re-normalize, bake/production.rs .\\-prefix special case) are removed. A new Bun.build-driven test asserts every manifest input/path is backslash-free and not absolute/..-escaping.
Security risks
None. This is output-path formatting for build artifacts; no user-controlled parsing, auth, or filesystem-escape surface is touched. The producer emits a posix-relative path derived from paths already validated by the bundler; consumers that write to disk go through bun_sys openat wrappers under root_dir.
Level of scrutiny
Moderate-high. The per-site changes are mechanical and each individually small, but the producer-side change to dest_path's separator convention flows to every consumer of additional_output_files on Windows: chunk assembly, standalone-graph key lookup, bake module keys, disk writes, HTTP route registration, and the CLI summary. The safety argument rests on Chunk.final_rel_path already carrying the identical invariant through the same consumers (verified), and on bun_sys openat/mkdirat normalising / before the NT syscall (stated, and consistent with how chunk final_rel_path already works). The fix cannot be validated by the Linux fail-before gate; Windows CI is the direct proof.
Other factors
The PR follows REVIEW.md's "fix at the layer that owns the invariant" guidance precisely — moving normalization to the producer and deleting three consumer workarounds that each carried a comment pointing at the others. Both CodeRabbit findings (disposable tempDir, assert every manifest entry) were addressed in follow-up commits. The new test is well-shaped: exercises a nested directory + file loader so both the input and file-loader path regressions are covered, and asserts on the full entry list rather than a per-loader sample. Deferring only because the Windows-only surface across three subsystems merits a maintainer confirming Windows CI, not because anything looks wrong.
|
@robobun fix clippy |
|
The clippy failure on 08930b7 was a transient fetch flake: |
|
Still reproduces at current main (1cf8af0, 1.4.0-canary) and in release 1.4.0 on Windows Server 2019, so this is still needed. The branch conflicts with main now ( Two more places the same
|
Problem
On Windows, importing an HTML file from JS and bundling for
target: "bun"emits a manifest whosefiles[].inputand file-loaderfiles[].pathfields are wrong:The manifest is meant to be handed to a static file server, so leaking a build-machine absolute path (and using
\separators) makes it unusable on Windows builds. This is the path-format symptom reported in #21283.Cause
Two separate gaps.
input:HTMLImportManifest::writederives the key withBoth operands are native absolute paths (
C:\...with backslashes on Windows).relative_normalizeddoes not normalise its inputs, and underPlatform::Posix\is not a separator andC:\...is not absolute, so each side is treated as a single path segment. The result is"../" + source.path.text.path:additional_output_files[].dest_pathis produced byPathTemplate::print, which emits native separators.Chunk.final_rel_pathis posix-converted right after templating (generateChunksInParallel.rs:372), but the parallel code for additional output files atbundle_v2.rs:4154-4158skips that step. Three consumers had already independently re-normalised the same field to work around this:Chunk.rs:946-956copied into a scratch buffer and posix-converted before emitting an asset URL;StandaloneModuleGraph.rs:778-786carried a#[cfg(windows)] platform_to_posix_bufwith a comment pointing back at theChunk.rsworkaround;bake/production.rs:273-279,757-763special-cased a.\prefix when buildingbake:/module keys.Other readers (static-route registration in
HTMLBundle.rs, asset URLs inbake/production.rs, the CLI summary inbuild_command.rs:1059) emitted the\form on Windows.Fix
inputwithrelative_platform_buf::<platform::Auto, true>(host semantics, normalised inputs) into a pooled buffer andplatform_to_posix_in_placethe result, matchingfinal_rel_path/ sourcemapsources.additional_output_files[].dest_pathonce at the producer, immediately aftertemplate.print(...)inbundle_v2.rs, so it carries the same invariant asChunk.final_rel_path. Every reader either already re-normalised, routes throughplatform::Auto/Loosepath helpers whose separator test accepts/, or wants a URL/module key where/is required; Windowsopenat/mkdiratnormalise/before the NT syscall.Chunk.rs,StandaloneModuleGraph.rs, andbake/production.rs.All path helpers touched are no-ops on POSIX, so Linux/macOS output is byte-identical.
Verification
New
html-import/manifest-paths-are-posix-relativeintest/bundler/html-import-manifest.test.tsbuilds withBun.build(so it runs on Windows today, independent of theitBundledharness) and asserts everyinput/pathis/-separated and project-relative.Windows x64:
Also verified on Windows with this branch:
bun build entry.ts --outdir out --asset-naming "[dir]/[name]-[hash].[ext]"emitsvar icon_default = "./sub/icon-neqd1z7a.txt"and printssub/icon-neqd1z7a.txtin the CLI summary (was.\sub\...);bun build --compile entry.tswith a file-loader asset embeds and reads the asset correctly (exercises theStandaloneModuleGraphkey).Linux x64: all 6 tests in
test/bundler/html-import-manifest.test.tspass with the two inline snapshots unchanged;bundler_loader.test.tsandbundler_naming.test.tspass.The fix only affects Windows output, so the Linux-only fail-before gate will see the new test pass both ways; Windows CI is the direct proof.
Note on the
itBundledsnapshot tests#34552 re-enables
itBundledon Windows and gateshtml-import/manifest-with-metadataandhtml-import/multiple-manifestsastodo: isWindows. With this fix applied those tests still fail there, because the harness's temp root is an 8.3 short path (C:\Users\AZUREU~1\...) whileoptions.root_diris canonicalised to the long form viaGetFinalPathNameByHandleW, sorelative(root_dir, source)walks up and back down. That is the sameroot_dir-vs-source canonicalisation mismatch that breaksnaming/AssetNamingDirand is tracked separately; once it is fixed thetodo: isWindowsgates on these two tests can be dropped.