resolver: fix sideEffects glob matching on Windows

[robobun]

What does this PR do?

Fixes #30320: on Windows, sideEffects globs never matched and every file in a package was treated as side-effect-free. prebid.js@10.29.0 ("sideEffects": ["dist/src/modules/**/*.js"]) silently lost every bid adapter on Windows.

Cause. Patterns were built with r_fs.join({package_dir, name}) (.loose platform). That routes through join_string_buf → normalize_string_node_t, which prepends a leading / for absolute inputs, so a Windows package dir produced /C:/proj/node_modules/my-lib/adapters/**/*.js. Runtime paths instead go through r_fs.abs → _join_abs_string_buf_windows, emitting C:\proj\...\foo.js — no leading /. After the shared \ → / pass in normalize_path_for_glob, the stored pattern kept a leading / the subject never had, so neither glob nor exact-match keys matched. The same mismatch broke exact-match keys, which is why four existing tests were marked todo: isWindows.

Fix.

1. Build the pattern with r_fs.abs instead of r_fs.join so it shares the joiner the runtime path uses.
2. Strip a package-root-relative leading / or \ from each entry before joining, so r_fs.abs joins against the package dir instead of discarding it (path.resolve treats an absolute later component as a new root). Without this, "/index.js"-style entries would regress on POSIX where r_fs.join previously concatenated them; matches esbuild's filepath.Join.
3. Normalize exact-match map keys at parse time and normalize the lookup path in has_side_effects (Map/Mixed), so \ vs / no longer affects the hash.
4. Route the Map/Glob/Mixed branches in finalize_result through has_side_effects so the lookup-side normalization always applies.
5. Drop the todo: isWindows markers on the four PackageJsonSideEffectsArray* bundler tests this now unsticks.

How did you verify your code works?

The bug is Windows-only (POSIX paths never gain the leading /), so test/regression/issue/30320.test.ts drives SideEffects::has_side_effects through bun:internal-for-testing with synthetic C:\pkg\... strings on any host. usePreFix: true reproduces the pre-fix r_fs.join shape, so the exact and mixed cases fail without the src fix and pass with it on Linux (verified locally: reverting the resolver diff makes 2/6 fail, restoring makes all pass). Two end-to-end bun build cases guard the real bundler path (glob entries, and a leading-slash entry). Windows CI exercises the original repro directly, and @brynne8 confirmed the build works on Win11.

Closes #30320
Closes #22598

[robobun]

^{Updated 3:39 AM PT - Jul 2nd, 2026}

❌ @robobun, your commit 7ea056d has 3 failures in Build #67878 (All Failures):

• test/js/bun/terminal/terminal.test.ts - code 1 on 🍎 14 x64
• test/js/web/fetch/body-stream.test.ts - timeout on 🐧 13 aarch64
• test/js/web/fetch/body-stream.test.ts - timeout on 🐧 13 x64
• test/js/web/fetch/body-stream.test.ts - timeout on 🐧 3.23 aarch64
• test/js/web/fetch/body-stream.test.ts - timeout on 🐧 13 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🪟 2019 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🪟 2019 x64
• test/bake/dev/production.test.ts - code 1 on 🪟 11 aarch64
• test/bake/dev/production.test.ts - code 1 on 🐧 13 x64-asan
• test/bake/dev/production.test.ts - code 1 on 🐧 25.04 aarch64
• test/bake/dev/production.test.ts - code 1 on 🐧 25.04 x64
• test/bake/dev/production.test.ts - code 1 on 🐧 13 aarch64
• test/bake/dev/production.test.ts - code 1 on 🐧 13 x64
• test/bake/dev/production.test.ts - code 1 on 🐧 3.23 aarch64
• test/bake/dev/production.test.ts - code 1 on 🐧 25.04 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🐧 13 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🐧 3.23 x64
• test/bake/dev/production.test.ts - code 1 on 🐧 3.23 x64-baseline
• test/bake/dev/production.test.ts - code 1 on 🍎 14 aarch64
• test/bake/dev/production.test.ts - code 1 on 🍎 14 x64

---

🧪   To try this PR locally:

bunx bun-pr 30322

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

bun-30322 --bun

[coderabbitai]

Walkthrough

Paths and sideEffects patterns are normalized and converted to absolute patterns for cross-platform matching; sideEffects checks are centralized; JS/JSC testing hooks added; unit and end-to-end tests for Windows path/glob scenarios and DCE cases were introduced or expanded.

Changes

Cross-Platform sideEffects Matching & Tests

Layer / File(s)	Summary
API / Data Shape src/resolver/package_json.zig	Made normalizePathForGlob public and added buildAbsolutePattern / buildAbsolutePatternPreFix public signatures to produce normalized absolute patterns.
Core matching logic src/resolver/package_json.zig	Normalize incoming runtime paths before map, exact-match, and glob checks; check normalized exact matches before glob evaluation; construct absolute patterns via r.fs.abs; normalize patterns; skip CSS files during pattern collection; use normalized patterns as keys when populating maps/lists.
Resolver wiring src/resolver/resolver.zig	Unified package.json-derived and existing-package side-effects handling by collapsing per-branch map/glob/mixed checks into a single hasSideEffects(path.text) decision path.
JSC / JS testing surface src/resolver_jsc/package_json_jsc.zig, src/js/internal-for-testing.ts	Added TestingAPIs.sideEffectsHasSideEffects (JSC) implementation and exported packageJsonInternals.sideEffectsHasSideEffects JS binding to exercise sideEffects matching (dir, patterns, path, optional usePreFix → boolean).
Tests — unit and E2E test/regression/issue/30320.test.ts, test/bundler/esbuild/dce.test.ts	Added regression tests covering Windows-style absolute, "./"-prefixed, exact, and mixed sideEffects patterns; expanded DCE tests previously gated by Windows checks to full test cases.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing sideEffects glob matching on Windows, which aligns with the core bug fix in the changeset.
Linked Issues check	✅ Passed	The changeset addresses all coding requirements from #30320: pattern construction uses r.fs.abs for Windows consistency, exact-match sideEffects keys are normalized at store and lookup time, Windows-gated test markers are removed, and regression tests covering glob, prefix, exact, and mixed cases are added.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing Windows path handling in sideEffects glob matching: Zig resolver logic, normalization functions, test infrastructure, and regression tests. No unrelated refactoring or features detected.
Description check	✅ Passed	The PR description includes both required sections and clearly explains the change and verification steps.

---

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

[github-actions]

Found 1 issue this PR may fix:

1. import "temporal-polyfill/global" is tree-shaken and not included in the build #22598 - Windows: temporal-polyfill/global side-effectful import is tree-shaken away because sideEffects array entries (exact paths like ./global.esm.js) fail to match on Windows due to path normalization mismatch — exactly the class of bug this PR fixes.

If this is helpful, copy the block below into the PR description to auto-close this issue on merge.

Fixes #22598

🤖 Generated with Claude Code

[claude]

I didn't find any bugs, but this swaps r.fs.join → r.fs.abs in the resolver's sideEffects pattern construction, which subtly changes path-joining semantics on all platforms (not just Windows) and directly affects tree-shaking decisions — worth a human sanity-check, especially with Windows CI as the only real discriminator.

Extended reasoning...

Overview

This PR fixes Windows-only sideEffects glob/exact matching by (1) building stored patterns with r.fs.abs instead of r.fs.join so they go through the same _joinAbsStringBufWindows joiner as runtime paths, (2) running exact-match map keys through normalizePathForGlob at both store and lookup time so \\ vs / doesn't break the hash, and (3) collapsing the .map/.glob/.mixed arms in resolver.zig to all route through hasSideEffects(). Four todo: isWindows markers are removed from existing dce tests, and a 4-case regression test is added.

Security risks

None. This is internal bundler path-normalization logic for tree-shaking decisions; no auth, crypto, or untrusted-input parsing is involved.

Level of scrutiny

Medium-high. The diff is small and the reasoning in the PR description is thorough, but:

• r.fs.join → r.fs.abs changes which path-joining codepath runs on every platform (joinStringBuf → joinAbsString with top_level_dir as cwd). On posix the first part (dirWithTrailingSlash()) is already absolute so the result should be identical, but this is exactly the kind of subtle path-handling change where edge cases (symlinks, ./ prefixes, trailing slashes) bite.
• Tree-shaking correctness is high-stakes: a false negative silently drops side-effectful code (the original bug); a false positive silently bloats bundles.
• The fix is only verifiable on Windows CI, which the author acknowledges — the new regression tests pass on Linux both before and after.

Other factors

• r.fs.abs returns a pointer into a shared threadlocal buffer (parser_join_input_buffer); the result is immediately duped by normalizePathForGlob, so lifetime is fine in the success path. The catch pattern fallback would store a temp-buffer pointer, but that's pre-existing behavior and only triggers on OOM.
• The robobun CI comment shows widespread build-zig failures on commit 4f5f0f81; an autofix commit landed afterward, so this may already be resolved, but it's worth confirming green CI before merge.
• No CODEOWNERS match for src/resolver/.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 test/regression/issue/30320.test.ts:155-158 — nit: per CLAUDE.md ("Writing Tests"), expect(stdout)... should come before expect(exitCode).toBe(0) so a failure shows the missing output rather than just an exit-code mismatch. Move expect(exitCode).toBe(0) after the two expect(stdout).toContain(...) lines.

  Extended reasoning...

  What this is

  CLAUDE.md line 128 documents an explicit repo convention for spawned-process tests:

  When spawning processes, tests should expect(stdout).toBe(...) BEFORE expect(exitCode).toBe(0). This gives you a more useful error message on test failure.

  test/CLAUDE.md lines 48–50 show the canonical ordering: stderr → stdout → exitCode. In the new end-to-end test at test/regression/issue/30320.test.ts:155-158, the assertions are ordered stderr → exitCode → stdout, so the exitCode check runs before the two stdout content checks.

  Why it matters (and why it mostly doesn't)

  The point of the convention is diagnostic quality: when a spawned process fails, you want the assertion that prints the content (stdout/stderr) to fire first, so the test failure message shows what actually went wrong instead of just expected 0, received 1.

  To be fair, this test already gets most of that benefit — expect(stderr).toBe("") is asserted first, so a bun build failure that writes to stderr will surface the diagnostic before either exitCode or stdout is checked. And in the specific regression this test guards against (sideEffects glob fails to match → side-effect imports tree-shaken), the build still succeeds with exitCode === 0 and empty stderr, so execution reaches the stdout assertions regardless of where the exitCode line sits. The current ordering does not affect test correctness.

  The remaining gap is the (admittedly narrow) failure mode where bun build exits non-zero with nothing on stderr. With the current ordering you'd get expected 0, received 1 and never see what was on stdout; with the documented ordering you'd see the stdout content (or its absence) first.

  Step-by-step

  1. Suppose bun build exits with code 1, writes nothing to stderr, and writes a diagnostic to stdout (rare but possible).
  2. Line 155 expect(stderr).toBe("") passes.
  3. Line 156 expect(exitCode).toBe(0) fails with expected: 0, received: 1 — test stops here.
  4. The stdout content that would have explained the failure is never printed in the assertion diff.

  With the documented order (stdout before exitCode), step 3 would instead be expect(stdout).toContain("foo adapter registered"), which fails showing the actual stdout content.

  Fix

  Swap the lines so the order is stderr → stdout → stdout → exitCode:

  expect(stderr).toBe("");
  expect(stdout).toContain("foo adapter registered");
  expect(stdout).toContain("bar adapter registered");
  expect(exitCode).toBe(0);

  Severity

  Nit. This is a documented style convention rather than a functional bug, and the practical diagnostic benefit is small here because stderr is already checked first. Flagging only because it's an explicit CLAUDE.md rule and the fix is a trivial one-line move.

[coderabbitai]

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 `@test/regression/issue/30320.test.ts`:
- Around line 141-146: The test currently asserts stderr is exactly empty which
is flaky on ASAN; update the test around the spawn result (variables stdout,
stderr, exitCode) to remove expect(stderr).toBe(""), keep the stdout assertions
(expect(stdout).toContain("foo adapter registered") and
expect(stdout).toContain("bar adapter registered")), then assert the exit code
last (expect(exitCode).toBe(0)); if the exit code is non‑zero, dump or assert on
stderr to aid debugging (e.g., fail the test with stderr content) so stderr is
only examined when the process fails.

🪄 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: d0e472fe-b1d1-42b4-80a9-945503408f88

📥 Commits

Reviewing files that changed from the base of the PR and between 7aad16e and 50479a0.

📒 Files selected for processing (2)

• src/resolver_jsc/package_json_jsc.zig
• test/regression/issue/30320.test.ts

[RiskyMH]

@robobun rebase this for the new rust version of bun

[robobun]

Rebased onto the Rust port — applied the same fix to src/resolver/package_json.rs (r_fs.join → r_fs.abs in the 3 parse branches, normalized map keys at both parse time and in has_side_effects lookups), routed all Map/Glob/Mixed branches in resolver.rs::finalize_result through has_side_effects so the normalization is applied on the lookup side too.

Dropped the Zig TestingAPIs scaffolding since porting the testing helper to Rust was more cost than value — regression is guarded by the end-to-end bundler test + Windows CI.

[brynne8]

@robobun Thank you for your Rust port. It works on my Win11. Looking forward to see it merged.

[brynne8]

@Jarred-Sumner When you have time, could you please review this PR?

[robobun]

Rebased onto main. The rebase was non-trivial: main finished the Zig port and deleted src/resolver/package_json.zig / resolver.zig, so the old Zig-era commits no longer applied. I re-landed the fix as a single pure-Rust commit against the current tree and migrated the bun:internal-for-testing binding from the now-dead $newZigFunction to $newRustFunction (host fn lives in bun_jsc::resolver_jsc::side_effects_testing, registered in rustIdentifierPaths + dispatch_js2native).

No behavior change from the last reviewed version. Fail-before re-verified locally: reverting just the resolver diff makes the exact + mixed Windows-path cases fail (2/6), restoring it makes all 6 pass.

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟣 src/resolver/package_json.rs:1436-1444 — Pre-existing, non-blocking: this exact-only branch has no .css skip, while the mixed (line 1379) and glob-only (line 1419) branches both continue on bun_paths::extension(name) == b".css", so ["styles.css"] keeps the entry but ["styles.css", "lib/*.js"] silently drops it. The exact-only branch is arguably the correct one (esbuild doesn't special-case CSS in sideEffects, and dropping an entry the author explicitly listed is backwards), so the follow-up is probably to remove the skip from the other two arms rather than add it here — just flagging since this PR rewrites all three.

  Extended reasoning...

  What this is

  A pre-existing inconsistency between the three sibling sideEffects parse arms in PackageJSON::parse, surfaced because this PR rewrites all three. It is not introduced by this PR and should not block it.

  • Mixed branch (line 1379) and glob-only branch (line 1419) both have:

    if bun_paths::extension(name) == b".css" { continue; }

  • Exact-only branch (lines 1436–1444, this hunk) has no such skip.

  The first-pass classifier at lines 1353–1365 does not look at extensions, so a .css entry with no glob characters routes to the exact-only or mixed arm purely based on whether any other entry in the array contains a glob character.

  Step-by-step proof of the asymmetry

  Take a package at /p/node_modules/pkg/.

  Case A — "sideEffects": ["styles.css"]:

  1. First pass: styles.css has no *?[{ → has_exact = true, has_globs = false.
  2. Falls into the exact-only else arm at line 1432.
  3. No CSS check → r_fs.abs([dir, "styles.css"]) → normalized → inserted into map.
  4. SideEffects::Map contains /p/node_modules/pkg/styles.css; has_side_effects("/p/node_modules/pkg/styles.css") → true.

  Case B — "sideEffects": ["styles.css", "lib/*.js"]:

  1. First pass: styles.css → has_exact = true; lib/*.js → has_globs = true.
  2. Falls into the mixed arm at line 1370.
  3. Iteration hits styles.css → bun_paths::extension(b"styles.css") == b".css" → continue (line 1379–1381). Entry dropped.
  4. SideEffects::Mixed has an empty exact map (only the JS glob in globs); has_side_effects("/p/node_modules/pkg/styles.css") → false.

  So the same explicit styles.css entry is honoured or silently discarded depending on whether an unrelated sibling entry happens to contain a *.

  Which arm is correct?

  This branch — the one without the skip — is almost certainly the right one, and the comment is filed here only because the PR rewrites these exact lines:

  • esbuild (the reference implementation this code ports) does not special-case CSS in sideEffects parsing at all; it stores every array entry. The exact-only arm matches that.
  • Packages list *.css in sideEffects precisely because CSS imports are side-effectful (they inject styles) and must survive tree-shaking. Dropping an entry the author explicitly listed inverts the field's purpose.
  • Adding the skip here (i.e. "fixing" the inconsistency in the wrong direction) would turn ["styles.css"] into an empty map → has_side_effects returns false → the explicitly-listed CSS file is marked side-effect-free. That would be a regression, not a fix.

  So if anything is wrong, it's the skip at lines 1379 and 1419, not the absence of one here.

  Why this may be moot in practice

  Bun's bundler appears to route CSS through a separate chunking pipeline (css_entry_points / ast.css guards in ParseTask), so it's plausible has_side_effects() is never consulted for a .css path and the dropped map entry is dead weight either way. I haven't traced that exhaustively; if it holds, the asymmetry is harmless storage noise rather than an observable tree-shaking difference. That uncertainty is another reason this is filed as pre-existing/non-blocking rather than a defect.

  Why mention it on this PR at all

  The diff context shows the pre-PR exact-only branch also lacked the CSS skip, so the asymmetry predates this change entirely and is orthogonal to the Windows path-shape fix (#30320). It's raised only because this PR mechanically rewrites the bodies of all three arms in the same commit, so the author is already looking at every line involved — a cheap moment to either drop the two continues for esbuild parity or leave a note explaining why CSS is intentionally skipped there.

  Suggested follow-up (not for this PR)

  Remove the if bun_paths::extension(name) == b".css" { continue; } from the mixed and glob-only arms so all three agree with esbuild and with each other. Do not add the skip to the exact-only arm.

[robobun]

Re the .css skip asymmetry between the exact-only branch and the mixed/glob-only branches: confirmed it is pre-existing (it mirrors how parse was structured before this PR) and orthogonal to the Windows path-shape fix. Leaving it as-is here to keep the diff scoped to #30320.

Agree with your read that the exact-only arm (no skip) matches esbuild, so the correct follow-up would be to drop the .css continue from the mixed and glob-only arms rather than add one here. That is a behavioral change to CSS sideEffects handling with its own implications (and, as you note, has_side_effects may not even be consulted for .css paths given the separate CSS chunking pipeline), so it belongs in its own PR rather than riding along on this one.

[robobun]

Stale PR review: closing.

Main already has this fix. 49b9a62 (merged in #34552, shipped in v1.4.1) builds the sideEffects patterns with the native path syntax (src/resolver/package_json.rs:282), so they match the resolver's C:\... paths. I ran three repros on Windows x64: an exact entry with an exports map, the glob adapters/**/*.js, and the #22598 shape. Bun v1.4.0 drops the side-effect import in all three, and a build of main (6d504dd) keeps it in all three. If you hit this bug, upgrade to Bun v1.4.1 or later.

Reopen if this evidence is wrong.