bundler: key PathToSourceIndexMap on (namespace, path) for plugin module identity

[robobun]

What

Bun.build plugins that returned the same path from onResolve under two different namespaces collapsed into one module. Whichever namespace resolved first won for both imports, and the second namespace's onLoad never ran. The same collapse meant an onResolve result whose path equalled a real file on disk but carried a custom namespace was served the disk file instead of the namespaced onLoad.

Repro

const r = await Bun.build({
  entrypoints: [entry],
  plugins: [{ name: "p", setup(b) {
    b.onResolve({ filter: /^nsa:/ }, () => ({ path: "SAMEPATH", namespace: "nsa" }));
    b.onResolve({ filter: /^nsb:/ }, () => ({ path: "SAMEPATH", namespace: "nsb" }));
    b.onLoad({ filter: /.*/, namespace: "nsa" }, () => ({ contents: `export default "A"`, loader: "js" }));
    b.onLoad({ filter: /.*/, namespace: "nsb" }, () => ({ contents: `export default "B"`, loader: "js" }));
  }}],
});
// entry: import a from "nsa:m"; import b from "nsb:m"; console.log({a, b});
// before: {a:"A", b:"A"}   (nsb onLoad never runs)
// after:  {a:"A", b:"B"}

esbuild (0.28.1) and Bun's runtime Bun.plugin face both treat these as distinct modules.

Cause

PathToSourceIndexMap keyed on path.text alone. In the on_resolve ResolveValue::Success arm (bundle_v2.rs), the plugin's returned namespace was written onto the Path but never entered the dedup key, so the second namespace hit found_existing and reused the first namespace's source index.

Fix

Key PathToSourceIndexMap on (namespace, path), matching esbuild's plugin contract. File-namespace entries (every resolver / entry-point / file-map / dev-server path, which is virtually all of them) keep the bare path.text key, so the common case is unchanged. Non-file namespaces store namespace ++ NUL ++ text; NUL is disallowed in filesystem paths on every supported platform, so a file-namespace key cannot collide with a composite one.

ResolveQueue stays path-keyed: its two population sites (the file-map and disk-resolver arms of resolve_import_records) are file namespace by construction, and plugin onResolve results bypass that queue via enqueue_on_resolve_plugin_if_needed.

Tests

Added to test/bundler/bundler_plugin.test.ts:

• plugin/NamespaceSamePathDistinctModules: two namespaces, same path, both onLoads run, distinct outputs
• plugin/NamespaceShadowsDiskFile: custom namespace with path equal to a real disk file goes through the namespaced onLoad
• plugin/NamespaceSamePathSameNamespaceDedup: control; same (namespace, path) via two specifiers stays one module (onLoad fires once)

The first two fail on 1.4.0-canary and pass with this change; the dedup control passes on both.

---

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

[coderabbitai]

Warning

Review limit reached

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

Next review available in: 20 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: d4c256fd-d3f1-4bf9-ac8e-931a5e51dc11

📥 Commits

Reviewing files that changed from the base of the PR and between 2e06e09 and 481e89b.

📒 Files selected for processing (1)

• src/bundler/PathToSourceIndexMap.rs

Walkthrough

The bundler now preserves module namespaces with paths during lookup, registration, deduplication, import patching, and removal. Plugin tests verify distinct namespaces, custom namespace precedence, and identical namespace/path deduplication.

Changes

Namespace-aware module identity

Layer / File(s)	Summary
Path namespace contract src/bundler/PathToSourceIndexMap.rs, src/paths/lib.rs	PathLike exposes path namespaces. Path-map operations use namespace-aware keys, while empty and file namespaces retain file-path keys.
Namespace-aware resolution flow src/bundler/LinkerContext.rs, src/bundler/barrel_imports.rs, src/bundler/bundle_v2.rs, src/runtime/bake/dev_server/incremental_graph.rs	Resolution, barrel imports, HTML imports, runtime setup, server components, incremental updates, and import patching pass namespaces with paths. Deduplication and map registration use both values.
Namespace identity validation test/bundler/bundler_plugin.test.ts	Plugin tests verify separate namespace identities, custom namespace precedence over disk files, and same namespace/path deduplication.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely states the main change: namespace-aware PathToSourceIndexMap keys for plugin module identity.
Description check	✅ Passed	The description explains the bug, cause, fix, and tests, and it provides verification details despite using a different heading than the template.

---

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

[github-actions]

Found 2 issues this PR may fix:

1. bun test --isolate drops the plugin onResolve namespace for query-suffixed imports #33904 - Directly describes namespace being dropped when onResolve returns a custom namespace, causing the bundler to fall through to the file loader instead of calling the namespaced onLoad
2. bun plugin: plugin build.onLoad loads other plugins when it shouldn't (?) and the rest of build.onLoad plugins are not called. #9373 - Multiple plugins' onLoad handlers not firing when they handle the same underlying file path under different plugin contexts, consistent with dedup map collapsing entries by path.text alone

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

Fixes #33904
Fixes #9373

🤖 Generated with Claude Code

[claude]

Beyond the inline nit, I also checked: (1) no PathToSourceIndexMap call sites exist outside the six touched files (grep confirms Graph.rs/lib.rs only declare, never call); (2) the composite-key NUL separator can't collide with a bare file-namespace key — filesystem paths reject NUL on all supported platforms, and plugin namespaces containing NUL would only self-collide, not cross into file entries; (3) bun_paths::fs::Path is Copy, so the dedup_fallback value-type change in barrel_imports.rs is a straight substitution.

Extended reasoning...

The PR changes the bundler's module-identity dedup key from path.text to (namespace, path.text) across ~15 call sites in 5 source files, plus adds three targeted plugin tests. The mechanical threading looked consistent at every site — hardcoded b"" is used only where the path is file-namespace by construction (secondary_path, bun:wrap runtime, dev-server abs_path invalidation, tsconfig enqueue), and path.namespace is used everywhere a full Path is in hand. I'm not approving because this reworks core bundler module identity across enough call sites that a maintainer should confirm each namespace choice, but nothing looked wrong.

[claude]

No issues found after re-review of ceaa0ef (the is_file_namespace nit is addressed). Deferring to a human since this changes the bundler's module-identity key across ~15 call sites in bundle_v2.rs / LinkerContext.rs / barrel_imports.rs / incremental_graph.rs, and the tests were not run locally ("no test proof" — deferred to CI).

What was checked:

• Every updated get/put/get_or_put/remove site passes a namespace consistent with how the entry was inserted; the b"" sites (secondary_path, tsconfig, bun:wrap, dev-server on_file_deleted) are file-namespace by construction.
• No direct .map field access bypasses the new namespace-aware wrappers.
• process_resolve_queue uses value.path.namespace with the resolve-queue key; per the PR description that queue is file-namespace only, so no key/namespace mismatch.
• composite_key's NUL separator can't collide with a file-namespace bare-text key on any supported platform.

Extended reasoning...

Overview

Re-keys PathToSourceIndexMap from bare path.text to (namespace, text), so plugin onResolve results with the same path but distinct namespaces become distinct modules (matching esbuild). File-namespace entries (empty or "file") keep the bare-text key; other namespaces store namespace ++ NUL ++ text. All get/put/get_or_put/remove signatures gain a namespace: &[u8] param, forcing every call site (~15 across bundle_v2.rs, LinkerContext.rs, barrel_imports.rs, incremental_graph.rs) to be updated. Also lifts is_file_namespace into bun_paths::fs and delegates Path::is_file() to it (addressing my earlier nit). Three new itBundled tests cover distinct-namespace-same-path, namespace-shadows-disk-file, and same-namespace-dedup control.

Security risks

None. No untrusted-input parsing, auth, or crypto. The NUL-separator key encoding is an internal map key, not exposed to user code, and NUL is disallowed in filesystem paths on every supported platform, so a file-namespace bare key cannot collide with a composite one.

Level of scrutiny

High. Module identity is the load-bearing invariant of the bundler dedup graph — a mismatched namespace at any lookup site would either silently duplicate a module (dedup miss) or collapse two distinct modules (the bug this fixes, but at a different site). The signature change means the compiler catches every miss, but whether each call site passes the correct namespace requires per-site reasoning about where the path came from. I traced each one and they look right, but this is exactly the kind of change where a maintainer familiar with the resolve-queue / secondary-path / server-components / dev-server flows should confirm.

Other factors

• Tests not run locally: the PR footer says "no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI". CI green would raise confidence significantly.
• barrel_imports.rs: the dedup_fallback map now stores Path<'static> (Copy) instead of &'static [u8], threading namespace through to get_path. Mechanically correct; persist_barrel_export still keys the dev-server persistence on .text alone, which is unchanged behavior and only affects the dev-server barrel cache (separate from the source-index map).
• Perf: composite_key allocates a temporary Vec<u8> per non-file-namespace lookup. Non-file namespaces are plugin-only and rare; the file-namespace fast path is unchanged.
• Previous bot feedback (comment-cop, my is_file_namespace nit) has been addressed and resolved.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/bundler/barrel_imports.rs (1)

462-476: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider extracting the duplicated dedup-fallback/target-resolution logic.

Lines 462-476 and Lines 546-560 compute the same thing: pick resolved_path from dedup_fallback when the record IS_UNUSED, otherwise use ir.path, then resolve target/ir_target from ir.source_index or map.get().get_path(&resolved_path). Extract this into a small helper (e.g. fn resolve_ir_target(ir, dedup_fallback, path_to_source_index_map) -> Option<u32>) to avoid keeping the two copies in sync on future changes.

Also applies to: 546-560

🤖 Prompt for 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.

In `@src/bundler/barrel_imports.rs` around lines 462 - 476, Extract the duplicated
resolved-path and target lookup from the two affected call sites into a shared
helper such as resolve_ir_target, accepting the import record, dedup_fallback,
and path_to_source_index_map and returning Option<u32>. Preserve the existing
precedence: use ir.source_index when valid, otherwise resolve the deduplicated
path through the map, returning None when no target can be found. Replace both
inline blocks with calls to the helper.

🤖 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 `@src/bundler/PathToSourceIndexMap.rs`:
- Around line 41-104: Replace the NUL-delimited encoding in
PathToSourceIndexMap::composite_key with an unambiguous delimiter-free encoding
using a fixed-width namespace length prefix followed by namespace and text
bytes. Keep the file-namespace bare-text behavior unchanged, and ensure get,
put, get_or_put, and remove continue routing non-file namespaces through the
updated composite_key.

---

Outside diff comments:
In `@src/bundler/barrel_imports.rs`:
- Around line 462-476: Extract the duplicated resolved-path and target lookup
from the two affected call sites into a shared helper such as resolve_ir_target,
accepting the import record, dedup_fallback, and path_to_source_index_map and
returning Option<u32>. Preserve the existing precedence: use ir.source_index
when valid, otherwise resolve the deduplicated path through the map, returning
None when no target can be found. Replace both inline blocks with calls to the
helper.

🪄 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: 56430538-0743-4121-a9fb-64892681a051

📥 Commits

Reviewing files that changed from the base of the PR and between fe473ea and 2e06e09.

📒 Files selected for processing (7)

• src/bundler/LinkerContext.rs
• src/bundler/PathToSourceIndexMap.rs
• src/bundler/barrel_imports.rs
• src/bundler/bundle_v2.rs
• src/paths/lib.rs
• src/runtime/bake/dev_server/incremental_graph.rs
• test/bundler/bundler_plugin.test.ts

[claude]

Beyond the inline nit, I spot-checked the hardcoded b"" namespace call sites (secondary_path at bundle_v2.rs:2092, enqueue_file_from_dev_server_incremental_graph_invalidation at :2484/:2510, bun:wrap at :3142, incremental_graph.remove at :1564) — all are file-namespace by construction (resolver secondary paths, fs-watcher abs paths, fixed runtime sentinel), so no dedup key drift there.

Extended reasoning...

The one finding this run is a theoretical encoding-collision nit (file-namespace bare key vs composite key sharing one keyspace); it will post inline. Separately I verified that every call site the PR changed to pass a literal b"" namespace is provably file-namespace: secondary_path is populated from resolve_result.path_pair.secondary.text (disk resolver output), the dev-server invalidation entry point carries an explicit // TODO: plugins with non-file namespaces and is fed only fs-watcher absolute paths, bun:wrap is the fixed runtime sentinel (behavior unchanged), and incremental_graph::on_file_deleted receives filesystem-watcher paths. Recording so a human reviewer need not re-trace those.

[robobun]

CI status: the bundler change is green everywhere it runs. The only hard failure on builds 86262 and 86322 is darwin-14-aarch64-test-bun expiring before an agent picks it up (macOS fleet capacity, not a test failure). Every test failure in the annotations is tagged flaky and passed on retry; none touch the bundler or plugin code paths.

Local verification: bundler_plugin.test.ts (57 tests), bundler_barrel.test.ts (49), bundler_html.test.ts (22), bundler_edgecase.test.ts (117) and bun-build-api.test.ts all pass with the debug build. The three new namespace tests fail on stock bun and pass with this change.

Ready for review.