Conversation
|
Status: ready for review. Rebuilt on main (289cb2a) after #41230 landed the printed path fix that this PR started as. Two commits remain:
Build 112448 finished with 180 of 181 jobs passed. |
|
Updated 11:23 PM PT - Sep 7th, 2026
❌ @robobun, your commit 289cb2a has 1 failures in 🧪 To try this PR locally: bunx bun-pr 35053That installs a local version of the PR into your bun-35053 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this touches unsafe lifetime-erased path storage in bundle_v2.rs plus the builtin BundlerPlugin.ts resolve flow, so it's worth a human look.
What was reviewed:
- Memory ownership in the new
externalbranch — the'static-erasedpathis only stored intoimport_record.pathon the arm that also parks both backing boxes onfree_list; every other arm drops the boxes only afterpathis dead. - The
{ external: true }no-path fallback inrunOnResolvePlugins— round-tripsinputPathto the native side, hits theeql_longequal-path arm, and leavesout_source_index = Noneso the record stays external without a rewrite. - The onResolve/parse race (import-records not yet populated) was raised and refuted — the new branch silently no-ops in that window rather than corrupting state.
Extended reasoning...
Overview
The PR fixes #2805: onResolve plugins returning { path, external: true } had the returned path discarded, so the emitted external import kept the original specifier. Three files change:
src/bundler/bundle_v2.rs— hoists theFs::Pathconstruction out of the!result.externalbranch and adds anelse ifarm that writes the plugin-returned path back into the importer'sImportRecord(mirroring the native resolver'sis_external_and_rewrite_import_pathhandling at ~L6278). BackingBox<[u8]>allocations are moved ontofree_listwhen the erased'staticslice is stored.src/js/builtins/BundlerPlugin.ts— moves theexternaltype check before the!pathshort-circuit, and falls back toinputPathwhenexternalis set with nopath(previously fell through tocontinue→ native resolve → "Could not resolve").test/bundler/bundler_plugin.test.ts— three newitBundledcases (ESM import rewrite, CJS require rewrite, external-without-path).
Security risks
None identified. This is bundler output-path rewriting driven by a user-supplied plugin; no auth, crypto, or filesystem-escape surface.
Level of scrutiny
Medium-high. The Rust change is small but sits inside an unsafe lifetime-erasure block with a hand-maintained SAFETY invariant (the free_list parking pattern). I traced each of the four terminal arms of the new else if and confirmed the 'static path is either (a) stored into import_record.path on the same arm that pushes both boxes onto free_list, or (b) dead before the boxes drop. The path.namespace field is either the b"file" literal or result_ns_static, and in the stored case both boxes go to free_list, so the namespace slice is kept alive too. The updated SAFETY comment accurately reflects the new branch.
The builtin JS change is small but has one observable side effect: { external: <non-boolean> } with no path now throws a TypeError where it previously hit continue and fell through. That's arguably a correctness improvement (esbuild parity) but is a behavior change a maintainer should sign off on.
Other factors
- Finder agents raised, and verifiers refuted, a race where
on_resolvecompletes before the importer's parse populatesimport_records(thelen() <= import_record_indexguard). In that window the new code drops the boxes and skips the rewrite rather than deferring viaresolve_tasks_waiting_for_import_source_indexthe way theout_source_indexarm does. The verifiers concluded this is not reachable / not a bug; I'm noting it because the guard is the one arm without a deferred-apply, and a maintainer familiar with the resolve/parse ordering should confirm. - Tests look solid (assert both presence of the rewritten specifier and absence of the original), placed alongside existing plugin tests, with the issue URL comment.
- The
ImportKind::EntryPointBuildexclusion is correct — entry points have no importerImportRecordto rewrite.
Given the unsafe-Rust surface and the minor JS-side validation-order change, I'm deferring rather than approving.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughSummaryThe bundler now preserves external plugin paths, defers updates until importer records are available, and applies them after path-map processing. Plugin validation and regression tests cover ESM, dynamic imports, CommonJS, collisions, and missing paths. ChangesExternal resolution handling
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Plugin-provided external paths are now preserved in emitted imports, while pathless external results retain the original specifier. Regression coverage includes the relevant import forms and collision cases, with no remaining merge-blocking risk identified. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 passed)
Full details: Linked Issues checkExplanation The PR addresses external import path behavior, but linked issue [ Full details: Out of Scope Changes checkExplanation The implementation and tests are focused on external onResolve path handling. This scope is not explicitly covered by the linked issue [
Comment |
There was a problem hiding this comment.
Beyond the inline finding, I also checked: the new EntryPointBuild-external error branch is covered by the decrement_scan_counter_on_drop guard at the top of on_resolve, so it won't hang the build; and the free_list handling in the external rewrite mirrors the existing !found_existing branch (boxes parked only when import_record.path actually borrows them). The candidate concern about the rewrite being dropped when source_import_records.len() <= import_record_index was examined and ruled out.
Extended reasoning...
This PR touches unsafe Rust with 'static lifetime erasure and manual free_list ownership in BundleV2::on_resolve, plus adds new user-facing behavior (entry-point-external now errors; {external: true} without path now externalizes instead of falling through). The change looks correct and well-tested, but the memory-safety surface and API behavior change warrant a human look. The inline nit about test/regression/issue/29264.test.ts is the only finding.
There was a problem hiding this comment.
Thanks for addressing the 29264 test coverage — 9b8e0ca restores the NoMatch → run_resolver path so the original crash guard is still live. I didn't find further issues, but this touches bundler memory ownership (unsafe 'static erasure + free_list parking) and changes user-visible onResolve semantics ({ external: true } without path now succeeds instead of falling through to a resolve error), so a maintainer should sign off.
What was reviewed:
- New external branch mirrors the native resolver's
is_external_and_rewrite_import_pathpattern at bundle_v2.rs:6290-6297 (sameeql_longguard, samepath_as_staticwrite). free_listownership: boxes are only dropped on paths wherepathis dead; on the rewrite path they're parked and outlive the borrow.- Verifiers ruled out a race where onResolve completes before
import_recordsis populated — the new branch bounds-checks before indexing.
Extended reasoning...
Overview
The PR fixes #2805: onResolve plugins returning { path, external: true } had the returned path discarded, so the emitted external import kept the original specifier. It also fixes the adjacent case where { external: true } with no path produced a spurious "Could not resolve" error, and adds an explicit error when a plugin marks an entry point external (previously a chunks.len() > 0 panic). Four files touched: bundle_v2.rs (~50 lines net in on_resolve), BundlerPlugin.ts (reordered external type check + inputPath fallback), four new itBundled cases, and the 29264 regression test adapted to keep exercising NoMatch → run_resolver.
Security risks
None identified. This is bundler plugin dispatch — no auth, crypto, or untrusted-input parsing beyond what already exists. The path string is written verbatim into the output bundle as an import specifier; it's not used for filesystem access.
Level of scrutiny
Moderate-to-high. The Rust change hoists an existing unsafe 'static-erasure block out of the !external branch so both branches can build Fs::Path from the plugin-returned boxes, and adds a new consumer of the free_list ownership-parking pattern. I traced every branch: on the rewrite path (!eql_long) the boxes are pushed to free_list before the 'static borrow escapes into import_record.path; on the eql_long, bounds-check-fail, and EntryPointBuild branches the boxes are dropped and path is not read afterward. The write pattern (import_record.path = path_as_static(&path)) matches the native resolver's external rewrite at bundle_v2.rs:6290-6297. This looks correct, but unsafe lifetime erasure in bundler core warrants a maintainer's eyes.
Other factors
- Behavior change:
{ external: true }withoutpathpreviously fell through asNoMatch(→ native resolver → "Could not resolve"); it now externalizes the input specifier. This matches esbuild and is clearly the intended semantics, but it's a user-visible change to plugin API behavior. - Prior feedback addressed: my earlier inline note about 29264.test.ts becoming inert was fixed in 9b8e0ca — the author added
import "other"returningundefinedand verified the test panics again with the load-bearing store reverted. - Test coverage: ESM import, CJS
require, no-path, and entry-point-error are all covered; existingbundler_plugin.test.tsandbun-build-api.test.tsreported passing locally. - Ruled out: verifier agents examined whether the external rewrite could be silently dropped when
onResolvecompletes beforeimport_recordsis stored — the bounds check at 4713 handles that case (drops the boxes, no crash, no rewrite), and in normal operationimport_recordsis populated before deferred plugin tasks resolve.
|
I opened #35577 with an overlapping fix before noticing this one and have since reduced that PR to the docs clarification for #11652 only. A couple of things I ran into while working on the same code path, in case they're useful here:
|
…ld when no entry point survives The onResolve external arm duplicated the error that #35053 already adds, so it is dropped here. In its place the CLI and Bun.build drivers fail with "None of the entry points could be bundled" when parsing ends with an empty entry point list, so any remaining way of dropping every entry point without logging becomes a build error instead of indexing an empty chunk list. The builtin entry point message no longer suggests another target, since a builtin cannot be an entry point for any target.
|
Heads up: #39799 adds the |
…linking zero entry points (#39799) ### Problem - `bun build` and `Bun.build()` abort with `panic: index out of bounds: the len is 0 but the index is 0` in `generate_chunks_in_parallel` (`generateChunksInParallel.rs:64`, `chunks[0]`) when every entry point is dropped (Sentry BUN-3RAS). With a live entry point beside it, the build exits 0 and silently emits fewer outputs. - Three producers drop an entry point without a log entry, so the drivers link with `graph.entry_points` empty: (a) a result with every path disabled (`"browser": {"./a.ts": false}`, or `fs` / `node:*` under the browser target); (b) an onResolve plugin that returns `external: true` for it; (c) an over-long specifier, which `resolve_entry_point` returned before it logged. ### Fix - `resolve_entry_point` rejects a disabled result: `"./a.ts" is disabled due to "browser" field in package.json (entry point)` or `Cannot use Node.js builtin "fs" as an entry point`. This covers the CLI, `Bun.build()` and the plugin fallback. It makes two no-path arms unreachable, so they are deleted: the `Ok(None)` return in `enqueue_entry_item` (the old drop site) and the `Worker entry point is missing` arm in `web_worker.rs`. - `on_resolve` logs `The entry point "x" cannot be marked as external` (esbuild's error). The length guard now only skips the cache bust, so an over-long entry point logs `ModuleNotFound` like any missing one. - Backstop: both drivers fail with `None of the entry points could be bundled` when no entry point survives parsing. The linker's `debug_assert` stays. The CLI drivers check the log after `wait_for_parse()`, as the JS driver did, so no parse task is in flight at teardown. - Verified: `test/bundler/bundler_browser.test.ts`, `bundler_plugin.test.ts`, `bun-build-api.test.ts`, `test/js/web/workers/worker.test.ts` (new cases, all red on 1.4.0). Other suites: see notes. ### Background - A disabled module is how the resolver represents `"browser": false` and browser-stubbed builtins: `Result::path()` is `None` and an import of it becomes `{}`. An entry point has nothing to emit in that state. - `enqueue_entry_item` appends each resolved entry point to `graph.entry_points` (a plugin answer arrives in `on_resolve` instead). The drivers wait for parsing, fail if the log has errors, then link. The linker needs one entry point, so every drop has to log. Supersedes #38778 and #38391. Carries the entry point arm of #35053, whose import path rewrite is independent. <details><summary>Notes</summary> Repros on 1.4.0 (each exits 134, now exits 1 or returns `success: false` with one message): ```sh bun -e 'await Bun.build({entrypoints:["node:fs"]})' bun build node:fs bun build fs echo '{"browser":{"./a.ts":false}}' > package.json; bun build --target=browser ./a.ts bun -e 'await Bun.build({entrypoints:["./b.ts"], plugins:[{name:"x", setup(b){ b.onResolve({filter:/b\.ts$/}, a => ({path:a.path, external:true})) }}]})' bun -e 'await Bun.build({entrypoints:["a".repeat(5000)]})' ``` Local debug build only: three `terminate()` tests in `worker.test.ts` (message flood, preload with un-awaited `import()`, `fs.readFile` completions) fail in this container, and fail the same way with the unmodified main sources built here. `production > works with sourcemaps` in `test/bake/dev/production.test.ts` hits its 5 s budget here and passes in 5.07 s with a longer one, with the expected `oh no!` output. The release binary passes all four. None of them involve entry point resolution. Other suites run on the debug build: `bundler_edgecase`, `bundler_naming`, `bundler_html`, `cli`, `test/bake/dev-and-prod`, and the three bundler files above in full. 1.3.x had the same drops and returned `success: true, outputs: []`. The port added the bounds check, so the drop now aborts. Silent drop on 1.4.0: `bun build --target=browser ./a.ts ./b.ts --outdir=out` exits 0 and writes only `b.js`. Now it exits 1 with the `./a.ts` error and writes nothing. The plugin test and the browser tests pin this form too. Long directory form of (c): a cwd of 3835 bytes plus a 500 byte relative entry point (`top_level_dir + entry + 4 > MAX_PATH_BYTES`) aborts on 1.4.0 alone and is silently dropped next to a valid entry point. With this branch both report `ModuleNotFound resolving "./eee...js" (entry point)` from the CLI and from `Bun.build()`. The same entry point as a 4337 byte absolute path still aborts in `load_as_file` (`src/resolver/resolver.rs:5888`), the resolver overflow #39626 is for. A specifier longer than the buffer inside a package with a `browser` field aborts in `check_browser_map` (`resolver.rs:5108`), which #37532 is for. Neither is an entry point drop. Worker: the length guard also made `new Worker(longName)` fire its error event with `BuildMessage: undefined`. The worker test pins the message. Unchanged: `--external ./b.ts` or `external: ["*"]` on an entry point still bundles it (entry points are exempt from external patterns, #12734). An absolute entry point path is never looked up in the browser map, so the bake and dev server callers of `resolve_entry_point`, which pass absolute paths, cannot hit the new error. `node:path` under the browser target still bundles its polyfill. Backstop reachability: the only known route left is `bun build --target=bun bun:wrap` in a release build (the specifier collides with the runtime's `bun:wrap` map key, so `enqueue_entry_item` returns `Ok(None)`). A debug build trips `assert_file_path_is_absolute` on that input first, so the backstop has no debug-runnable test of its own. Builtin specifiers as entry points under `--target bun`/`node` are a separate, pre-existing problem and are reported separately. Teardown: `enqueue_entry_points_common` schedules the runtime parse task before any entry point is resolved. #38778 saw ASAN crashes in `Worker::deinit_soon` on the CLI error path while the drivers still returned before `wait_for_parse()`. With this branch under the ASAN debug build, 30/30 runs of `bun build --target=browser ./a.ts` exit 1 with the message, and 20/20 runs with two bad entry points report both errors. `USE_SYSTEM_BUN=1` (1.4.0): the two new bun-build-api tests fail (the child aborts), the plugin test fails (the child aborts), 4 of the 5 new bundler_browser cases fail (the `--target=bun` control passes both ways by design), the worker test fails with `BuildMessage: undefined`. #38752 (`--no-bundle`) keeps its own message in the transform path, which this change does not touch. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts test/bundler/bundler_plugin.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
9b8e0ca to
03bcf96
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
f14a1db addresses my last note — the map pass in patch_import_record_source_indices now skips IS_EXTERNAL, and the barrel-revisit test was extended so the external and the bundled virt module share the same path.text, which pins that case. I found no further issues this pass; given this reworks core bundler resolution (unsafe 'static erasure, a new ImportRecordFlags bit, PendingImport becoming an enum) and went through several real-bug iterations, a human look is still worthwhile.
What was reviewed:
on_resolveexternal arm: boxes parked onfree_listbeforepathis stored; SAFETY comment matches the four current arms.apply_or_defer_pending_importand the pending-drain loop:ExternalPathapplied before the map pass, map pass skipsIS_EXTERNAL, so a plugin path cannot be looked up as a bundled module in either the immediate or queued case.resolve_import_records: every in-loop external site now setsIS_EXTERNAL; the estimate and skip conditions include it.29264.test.tsstill exercisesNoMatch → run_resolvervia the added"other"import.
Extended reasoning...
Overview
The PR makes onResolve plugins that return { path, external: true } actually rewrite the emitted specifier (fixes #2805) and makes { external: true } without path externalize the original specifier instead of falling through to native resolution. It touches src/bundler/bundle_v2.rs (the on_resolve Success arm, resolve_import_records, patch_import_record_source_indices, and a new apply_or_defer_pending_import helper), adds ImportRecordFlags::IS_EXTERNAL in src/ast/import_record.rs, tweaks runOnResolvePlugins in src/js/builtins/BundlerPlugin.ts, and adds five bundler-plugin tests plus adjusts the #29264 regression test.
Security risks
None identified. The change is bundler resolution bookkeeping; the only unsafe is a hoist of an existing 'static lifetime erasure whose invariant (bytes parked on free_list before path is stored, or dropped without storing) I traced across all four arms.
Level of scrutiny
High. This is core bundler resolution: it introduces a new flag bit consumed by resolve_import_records and the path-map pass, converts PendingImport from a struct to an enum, and rewrites how external plugin answers are applied. Four prior review passes each found a real bug (inert regression test, silent drop on the not-yet-populated race, barrel-revisit re-resolving the rewritten specifier, and the map-pass collision), all of which the author fixed. That history argues for a human sign-off even though this pass found nothing.
Other factors
- f14a1db is exactly the fix my last comment asked for and also folds the deferred
ExternalPathapplication into the single pending loop (so the after-map block is gone), and theResolveExternalRewriteSurvivesBarrelRevisittest now shares"react-vendored"between the external and a bundledvirtmodule — that is the collision case. - The
IS_EXTERNALbit is distinct fromIS_EXTERNAL_WITHOUT_SIDE_EFFECTS(the author's rationale — a plugin external may have side effects — is sound); I checked that the linker's drop-if-unused path keys on the latter, not the former. - The PR body's Notes are candid about untested surface (the queued
ExternalPatharm is not reachable today;schedule_barrel_deferred_imports' extra synchronous re-resolve predates this PR).
### Problem
- A `sideEffects: false` barrel is resolved again, as a whole, when an
importer asks for a re-export without `source_index` or un-defers one of
its records. With no plugins, Bun 1.4.0 reports `Could not resolve:
"./missing.js"` twice for one broken re-export. An `onResolve` plugin
for an external re-export runs 2 to 3 times for one record.
- `schedule_barrel_deferred_imports`
(`src/bundler/barrel_imports.rs:852`, `:887` on main) reads a missing
`source_index` as "never resolved". An external, failed, or
plugin-pending record never gets one.
- `resolve_barrel_records` passes the whole list.
`resolve_import_records` (`src/bundler/bundle_v2.rs:6002`) skips only
records with a `source_index`.
### Fix
- The BFS resolves a barrel only when the current item un-deferred a
record in it, and passes those indices. The `barrels_to_resolve` map and
the final loop are gone.
- `ResolveImportRecordCtx` and `PatchImportRecordsCtx` get
`only_records`. Both loops skip every other record. It replaces
`force_save`, which covered the same case. No `ImportRecord` flag is
added.
- Correct because records are deferred before the barrel is resolved.
Every other record was resolved with the barrel, which also requested
its target. An un-deferred record is in the list and resolves as before.
- Verified: four new tests in `test/bundler/bundler_barrel.test.ts`,
each fails on main. Other suites in Notes.
### Background
- A barrel is a module of a `sideEffects: false` (or `optimizeImports`)
package whose exports are all re-exports. Before it is resolved,
`apply_barrel_optimization` marks the re-exports nobody asked for yet
`IS_UNUSED`: they are deferred.
- When a later file imports such a name,
`schedule_barrel_deferred_imports` clears the flag (un-defers) and calls
`resolve_barrel_records`: `resolve_import_records`, then
`patch_import_record_source_indices`, which writes the `source_index` of
each module found. The BFS follows it into the next barrel.
- Only a record that points at a module of the bundle gets a
`source_index`. An external record gets none, a failed one is disabled,
and an `onResolve` match is answered later by the JS thread.
<details><summary>Notes</summary>
Plugin-free repro. Bun 1.4.0 prints 2, this branch prints 1. `a.js` is
found through the barrel, so its request always arrives after the barrel
deferred `Broken` and `C`:
```sh
mkdir -p /tmp/b5/node_modules/lib && cd /tmp/b5
echo '{"name":"lib","main":"index.js","sideEffects":false}' > node_modules/lib/package.json
printf 'export { A } from "./a.js";\nexport { Broken } from "./missing.js";\nexport { C } from "./c.js";\n' > node_modules/lib/index.js
printf 'import { Broken, C } from "lib";\nexport const A = "a:" + Broken + C;\n' > node_modules/lib/a.js
echo 'export const C = "c";' > node_modules/lib/c.js
printf 'import { A } from "lib"; console.log(A);\n' > entry.js
bun -e 'const r = await Bun.build({ entrypoints: ["./entry.js"], throw: false }); console.log(r.logs.length)'
```
When the second importer is an ordinary file instead, the count depends
on whether that file is parsed before or after the barrel, so the same
build reports 1 or 2 errors from run to run.
Plugin repro: a barrel with `export { default as React } from "react"`
and an `onResolve` plugin that returns `{ path, external: true }`. The
plugin runs when the barrel is resolved, again from the barrel's own
`schedule_barrel_deferred_imports` call (the seeded request for `React`
finds no `source_index`), and again for each later importer. The plugin
case that needs the index list and not only the barrel change is a later
importer that un-defers a different record:
`barrel/ExternalReExportNotResolvedAgainOnUnDefer`.
The second pass could also fail. `export { x } from "bun:whatever"` in a
barrel, target `bun`, fails on main with `Could not resolve:
"whatever"`: the `bun:` arm strips the prefix, and the second pass
resolves the stripped name as a package. It builds with this change. No
test for it, the same pass is what the other tests pin.
Relation to #35053: its barrel test hits the same second pass and adds
`IS_EXTERNAL` so that pass skips its rewritten record. With this change
the pass does not reach that record. This PR takes no flag bit.
Dev server: import records there get no `source_index` from the normal
parse path, so before this change a request for a never-deferred record
also resolved the barrel again, and `force_save` wrote indices onto all
of its records. The work item that pass produced duplicated the request
the barrel's own call made in phase 1 (`barrel_imports.rs:462-510` and
`:550-586`) and was dropped by the `requested_exports` check.
Un-deferred records still get their indices written. The barrel tests in
`test/bake/dev/bundle.test.ts` pass.
Not changed: a plugin answer that arrives after the BFS does not request
anything from the module it resolves to. Same before and after.
The first revision of this PR used an
`ImportRecordFlags::RESOLVE_STARTED` bit instead of the index list. Bit
10 is the last free bit of the `u16`, and #35053 and #38461 both take
it, so the self-review asked for this shape. The barrel change alone
fixes two of the four tests. The index list is what fixes
`barrel/UnDeferReportsUnresolvableSiblingOnce` and
`barrel/ExternalReExportNotResolvedAgainOnUnDefer`.
Suites run on the debug build: `bundler_barrel`, `bundler_plugin`,
`bundler_plugin_chain`, `bundler_edgecase`, `bundler_npm`,
`bundler_browser`, `bundler_bun`, `bundler_cjs`, `bundler_splitting`,
`bundler_regressions`, `bundler_allow_unresolved`, `metafile`,
`bun-build-api`, `native-plugin`, `bake/dev/bundle`, `bake/dev/plugins`,
`bake/dev/esm`, `bake/dev/hot`, `regression/issue/29264`. With the
`src/` changes stashed, the new tests fail on the same debug build. The
plugin-free test failed 15 of 15 runs on Bun 1.4.0.
</details>
f14a1db to
af0a3e4
Compare
There was a problem hiding this comment.
Reviewed again after 1b41dce — no new findings. Both prior nits are addressed: ExternalPath::apply now sets source_index = Index::INVALID, every external answer (including the unchanged-specifier case) routes through it, and plugin/ResolveExternalClearsModuleMatchedBySpecifier{WithPath,WithoutPath} pin the clear. The PR description now matches the shipped mechanism. Given this touches core bundler resolve/patch logic with an unsafe 'static erasure, a human look is still worthwhile.
What was reviewed: the simplified external arm in on_resolve (all externals now park on free_list and go through apply_or_defer_pending_import); the updated SAFETY comment against the four remaining arms — accurate; ExternalPath::apply clearing a stale map-pass source_index in both immediate and deferred paths; the { external: true } no-path fallback in runOnResolvePlugins and its interaction with the entry-point error and 29264.test.ts.
Extended reasoning...
Overview
1b41dce is the only commit since my last review. It addresses both nits I left on 2026-08-22: (1) PendingImport::ExternalPath::apply now writes import_record.source_index = Index::INVALID so a stale index written by the initial map pass (from the raw specifier) is cleared; (2) the separate unchanged-specifier eql_long arm is gone — every non-entry-point external answer now parks the boxes on free_list and goes through ExternalPath, so the clear applies uniformly. Two new tests (ResolveExternalClearsModuleMatchedBySpecifier{WithPath,WithoutPath}) force the map-pass collision via an onLoad gate on late.js that waits for the virt answer, and assert the output has no __INVALID__REF__ / __toESM(,. The PR description was rewritten to describe #39874's only_records filtering instead of the dropped IS_EXTERNAL flag.
Security risks
None. No untrusted-input parsing, auth, or network surface. The unsafe block is the pre-existing 'static erasure of plugin-owned Box<[u8]>; I re-checked the SAFETY comment against the four remaining disposition sites (!found_existing → free_list; external non-entry → free_list before the path is stored in PendingImport; found_existing and EntryPointBuild → drop with path never stored) and it is accurate after the simplification. The free_list.push calls precede apply_or_defer_pending_import, so the queued Path<'static> borrows bytes owned by free_list for the graph's lifetime.
Level of scrutiny
High. This is core bundler resolve/patch logic in bundle_v2.rs with unsafe lifetime erasure, and the PR history shows four separate real issues found and fixed across review rounds (silent race drop, barrel-revisit re-resolve, map-pass collision on rewritten path, stale source_index from raw specifier). Each was confirmed by the author with a failing test. That subtlety is why I'm deferring rather than approving despite a clean bug-hunter run.
Other factors
Test coverage is thorough for the feature (static/re-export/star/dynamic import, require, no-path, same-path, barrel revisit, specifier-keyed map collision × 2). The 29264.test.ts adjustment keeps the original NoMatch → run_resolver guard live via import "other", verified by the author against the original panic. All prior review threads are resolved by code changes; the two unresolved inline comments from my last run are addressed by 1b41dce even though no reply was posted.
### Problem - GitHub closes only the first reference after a keyword, so "Fixes #1, #2" leaves #2 open. "Supersedes #3" links nothing, and no reference closes a pull request. - The last 1000 merged PRs name 274 such references. PR #32292 is open although merged #36135 says "Supersedes #32292". ### Fix - `.github/workflows/close-linked-issues.yml` runs on `pull_request_target` `closed` (a merge into the default branch of `oven-sh/bun`) and on `workflow_dispatch` with a PR number and `dry_run`. Everything is inline in one `actions/github-script` step, with no checkout. - Each open target is closed as `completed` with the comment "Closed as completed by #N." or "Superseded by #N.". Closed or missing targets, the PR itself and other repositories are skipped. - The parser has no regex. A closing keyword (close, fix, resolve, supersede, replace, any tense) must lead the reference, alone or in a list. A negated, hedged or noun keyword, or one whose subject is another reference, does not count ("may fix", "the rm fix #1", "#100 supersedes #1"). - Verified: `test/internal/close-linked-issues.test.ts` (333 cases) runs the YAML's script against fake `github`, `context` and `core`. Also the 1000-PR parse (Notes). ### Background - GitHub's own keywords are close, fix and resolve (-s, -ed). Each links one reference, and only a merge into the default branch closes it. - `pull_request_target` runs in the base repository with a write token, also for fork PRs. That is safe only when no PR-controlled code runs. Here the description is the only PR input, parsed as text. <details><summary>Notes</summary> A close through the API does not create the "closed this in #N" timeline link that GitHub makes for its own closes. The comment carries the PR number instead. How the parser was calibrated. I pulled the descriptions of the last 1000 merged PRs and listed every line with a keyword next to a reference. The keyword families, list shapes and reference forms in the script are the ones that appear there. A reference is `#1`, `owner/repo#1`, an issue or pull URL (bare or in `<>`), or a markdown link. Four lines would have been wrong with a plain keyword-then-reference rule, and each led to a rule: - "the open `rm` fix #37521" (#38379): "fix" as a noun. Base forms (fix, close, resolve, supersede, replace) count only at the start of a sentence or line, or after will, should, does, and, and a few similar words. "to" is not one of them ("unable to fix #1", "how to fix #1"). - "May also fix #12318 / #10046, untested" (#38242): hedged. may, might, could, would, partially and the negations disqualify the keyword, looking past adverbs such as "also". - "Supersedes the closed #26040" (#36289) and "a comment on closed #35351" (#35365): "closed" as an adjective. A determiner or preposition before the keyword disqualifies it. - "supersedes #33130's optimisation" (#35843): a number that continues into a word is not a reference. Review added: a reference before the keyword is the subject ("#100 supersedes #1"), also through "which" or "that" ("reverts #100, which fixed #1") and across a removed span ("#100 ~~also~~ fixes #1"). A hedge two words before the keyword disqualifies it ("hopefully this fixes #1", "could this fix #1?"). A clause that starts with if, when, once, until or unless is not a statement. The tokenizer keeps a line break as a token so that "Fixes #1" on one line and "Fixes #2" on the next stay two statements. Code spans, fences, indented code, blockquotes, HTML comments and strikethrough are skipped. The block stripping follows CommonMark for fences (also inside a blockquote), indented code, blockquotes with lazy continuation, setext underlines and HTML comments, and GFM for `~~` flanking. Result over the 1000 descriptions: 274 distinct references in 135 PRs. I checked the current state of all of them through GraphQL. All but one are closed (202 issues completed, 5 duplicates, 66 pull requests). The one open target is PR #32292, superseded by merged #36135. No open target is a false positive. Every review change kept this result. Patterns that are deliberately not handled: a bulleted list under "Closes:" on its own line (not seen in the sample), references separated by whitespace only ("#1 #2"), "fix for #1", and GH-1 style references. A `?` after the list is not treated as a question. The block parser tracks no list containers, so a second paragraph of a list item indented by four spaces is read as an indented code block and skipped. A removed span or inline comment reads as one word, so "Fixes <!-- n --> #1" finds nothing. The test suite covers: the phrases above, stopping at the right place in real sentences, CRLF descriptions, URLs with fragments or a `/files` suffix, case-insensitive `Owner/Repo#1`, the fake API where a lookup, an update or a comment fails, the `dry_run` input, an invalid `pr_number` input, an unmerged PR, a PR merged into a non-default branch, the merge event body against a later edit, and a description with no closing statement. The first revision of this PR checked out the repository and ran `scripts/close-linked-issues.ts`. Jarred asked for no checkout and no script file, so the script moved inline into the workflow and the test now reads it out of the YAML. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 9 · 2 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/internal/close-linked-issues.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/internal/close-linked-issues.test.ts bun test v1.4.1 (4448a2e) test/internal/close-linked-issues.test.ts: (pass) finds "Fixes #39852" [176.21ms] (pass) finds "Closes #31772. Fixes #31771." [22.28ms] (pass) finds "- Fixes #39930" [12.28ms] (pass) finds "Fixes: #30429" [10.46ms] (pass) finds "FIXES #1" [7.86ms] (pass) finds "(Fixes #1)" [8.97ms] (pass) finds "**Fixes #1**" [10.20ms] (pass) finds "__Fixes #1__" [9.83ms] (pass) finds "_Fixes #1_" [11.25ms] (pass) finds "Fixes **#1**" [9.72ms] (pass) finds "**Fixes** #1" [7.13ms] (pass) finds "**Fixes:** #1" [8.11ms] (pass) finds "Fixes #1 and **#2**" [11.47ms] (pass) finds "Fixes **#1**, **#2**" [9.13ms] (pass) finds "## Why (fixes #13771, closes #30543)" [16.08ms] (pass) finds "Closes #11418" [19.46ms] (pass) finds "Resolves #1. Resolved #2. Resolve #3." [12.09ms] (pass) finds "Fixes #34055, #30327, #24394, #20816, #32403, #11898, #10056." [17.11ms] (pass) finds "Fixes #18192 and #31675 as a consequence" [10.45ms] (pass) finds "Fixes #1, #2, and #3" [10.96ms] (pass) finds "Fixes #1 & #2" [7.63ms] (pass) finds "Closes #33280, Closes #32864 and Closes #29696 (the timer in #32949 is orthogonal)" [20.29ms] (pass) finds "Closes #33182 and #32947 on top of current main (which already has #36304 for catalogs)." [16.12ms] (pass) finds "Fixes #1,\n#2" [7.76ms] (pass) finds "Fixes #1, #2,\nand #3" [9.27ms] (pass) finds "Fixes #1\nand #2" [8.57ms] (pass) finds "Fixes #1\n& #2" [6.80ms] (pass) finds "Fixes #1 and\n#2" [7.31ms] (pass) finds "Supersedes #39908 (same change, moved from a fork branch)" [13.21ms] (pass) finds "Supersedes #38778 and #38391. Carries the entry point arm of #35053." [14.43ms] (pass) finds "Supersedes #39193 and keeps its three tests." [11.48ms] (pass) finds "This supersedes #33306 and #32803. Their tests are kept here." [13.73ms] (pass) finds "- This replaces #33793. Its ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` .github/workflows/close-linked-issues.yml | 950 ++++++++++++++++++++++++++++++ test/internal/close-linked-issues.test.ts | 598 +++++++++++++++++++ 2 files changed, 1548 insertions(+) ``` </details> **gate history** · 29 passed · 0 rejected · iteration 9 <details><summary>evidence per changed file</summary> ``` file reads edits tests .github/workflows/close-linked-issues.yml 6 12 0 test/internal/close-linked-issues.test.ts 3 11 0 ``` </details> <!-- robobun:evidence:end -->
1b41dce to
deab78a
Compare
…41230) ### Problem - An `onResolve` result of `{ path, external: true }` keeps the import external, but the output prints the original specifier. For `import { v } from "./b.js"` and `path: "/somewhere/else/b.js"`, `Bun.build` emits `require("./b.js")` (cjs) or `from "./b.js"` (esm). esbuild emits the returned path. - The external arm of `BundleV2::on_resolve` (`src/bundler/bundle_v2.rs:5035` on main) drops `result.path`. It never updates the import record, and the printer emits the record's path. ### Fix - The external arm stores the returned path on the import record. The path bytes move into `free_list`, like the bytes of a bundled plugin path. - The path construction moves above the `external` check, so both arms share one lifetime erasure. The entry point error is unchanged. - Correct because the record still has no `source_index`, so it stays external. Only the printed text changes. - Verified: `test/bundler/bundler_plugin.test.ts`, four new `plugin/ResolveExternalRewrites*` cases and two `plugin/RewriteExternal*` cases from esbuild. All six fail on 1.4.1. Also ran the barrel, plugin chain and dev server plugin suites. ### Background - An import record is one `import`, `require` or `export from` in a parsed file. A record with no `source_index` is external, and the printer emits its `path.text`. - An `onResolve` answer runs on the bundle thread as a posted task. `on_parse_task_complete` stores the importer's records before that task can run. - `free_list` holds plugin-owned byte buffers that the graph points into. `deinit_without_freeing_arena` frees them. <details><summary>Notes</summary> - This is the path fix from #35053, alone. #35053 also externalizes `{ external: true }` with no path, clears a `source_index` that the path map set from the specifier, and extends the pending import queue. This PR does none of those. - The new arm indexes the importer's records directly. `run_resolver` does the same for a plugin answer with no match. `resolve_tasks_waiting_for_import_source_index` is not reached today: it dates from 994c715, when resolution ran on worker threads. This PR leaves it alone. - The record takes the namespace of the result, as a bundled plugin path does. The bundler never prints a namespace for an external record, so the output does not depend on it. - Metafile: the import now reports `"path": "/somewhere/else/b.js"` and `"original": "./b.js"`. - The `plugin/RewriteExternal*` cases are `rewriteExternalWithNamespace` and `rewriteExternalWithoutNamespace` from esbuild's [`scripts/plugin-tests.js`](https://github.com/evanw/esbuild/blob/f6058f8364fe7ab91ca57a83e02577ed74c9cae4/scripts/plugin-tests.js#L680-L760). The third case there, `rewriteExternalWithFileNamespace`, is not translated. With an explicit `namespace: "file"`, esbuild writes the path relative to the output directory. Bun's plugin runner sets the namespace to `"file"` when the plugin omits it, so the bundler cannot tell the two cases apart. esbuild's Go bundler tests have no external `onResolve` case. - Suites run on the debug build: `bundler_plugin`, `bundler_plugin_chain`, `bundler_barrel`, `regression/issue/29264`, `regression/issue/40606`, `regression/issue/bundler-plugin-onresolve-entrypoint`, `bake/dev/plugins`. </details> <!-- robobun:evidence:begin --> --- **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_plugin.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
…th no path
`{ external: true }` with no `path` hit `if (!path) continue` in
runOnResolvePlugins, so the answer was ignored and the native resolver
ran, usually failing with "Could not resolve". esbuild keeps the import
external under its original specifier. Fall back to the input path when
`external` is set, and validate `external` before that check.
test/regression/issue/29264.test.ts relied on the old fall-through: its
catch-all filter also answered for the entry point, which now reaches the
"cannot be marked as external" error. The plugin skips entry points, and
a third bare import that the plugin declines keeps the NoMatch ->
run_resolver path that #29264 guards exercised.
…the specifier
The path map is keyed by path text alone, and the map pass in
patch_import_record_source_indices runs over records whose plugin answer
is still pending. When an earlier plugin answer bundled a module whose
path text equals a later import's specifier, that import picked up the
module's source_index, and the external answer left it in place. The
linker then treated the import as that module. Released Bun prints
var __INVALID__REF__ = __commonJS(function(exports) {});
var import_react = __toESM(, 1);
and the debug build asserts `!ref_.is_empty()` in
print_code_for_file_in_chunk_js.
The external arm of on_resolve now resets source_index when it stores the
returned path. Two tests pin the interleaving with an onLoad gate, one per
answer shape. Two more cover cases found while reviewing earlier revisions
of this change and pass on main: an answer that returns the specifier
unchanged, and a rewritten external re-export in a sideEffects:false
barrel that a later consumer un-defers next to a bundled module sharing
its path text.
90a9587 to
289cb2a
Compare
robobun
changed the title
{ external: true } with no path, and clear a stale source_index on external answers
This branch has not been deployed
2 participants
Problem
onResolveanswer of{ external: true }with nopathis ignored.runOnResolvePluginshitsif (!path) continue(src/js/builtins/BundlerPlugin.ts:444on main), the native resolver runs, and the build fails withCould not resolve: "lodash". esbuild keeps the import external under its original specifier.onResolveanswer can leave the import linked to a bundled module. The path map is keyed by path text, and its pass inpatch_import_record_source_indicesruns while a plugin answer is pending. If an earlier answer bundled a module whose path text equals this import's specifier, the import gets that module'ssource_index, and the external arm ofon_resolve(src/bundler/bundle_v2.rs:5089on main) never clears it. Released Bun printsvar __INVALID__REF__ = __commonJS(function(exports) {});and__toESM(, 1). The debug build asserts!ref_.is_empty()inprint_code_for_file_in_chunk_js.Fix
runOnResolvePluginsfalls back to the input specifier whenexternalis set andpathis empty, and validatesexternalbefore that check.on_resolvesetssource_index = Index::INVALIDafter it stores the returned path. An external record is one with nosource_index, so this is the same invariant the arm already relies on.test/regression/issue/29264.test.tsrelied on the old fall-through: its catch-all filter also answered for the entry point, which now reaches the entry point error. The plugin skips entry points, and a third bare import that the plugin declines keeps theNoMatch -> run_resolverpath that Bun segfaults when there are both external and unresolved/missing non-external imports #29264 guards live. Verified by commenting out the error-path store: the test panics inrun_resolveragain.test/bundler/bundler_plugin.test.ts,plugin/ResolveExternalWithoutPathandplugin/ResolveExternalClearsModuleMatchedBySpecifier{WithPath,WithoutPath}fail on main and pass here. Alsobundler_barrel,bundler_plugin_chain,bundler_edgecase,bake/dev/plugins,29264,40606.Background
onResolveanswers run on the bundle thread as posted tasks, afteron_parse_task_completehas stored the importer's records and run the path map pass over them. That pass gives every record whosepath.textis a key inpath_to_source_index_mapasource_index. Bundled plugin answers insert their returned path text into that map.source_indexclear.Notes
ResolveExternalClearsModuleMatchedBySpecifier*:entry.jsimportsvirt, which the plugin bundles at path textreactin namespacevirt.late.jsis loaded throughonLoadonly after that answer is queued, and importsreact, which the plugin externalizes. So the map pass overlate.jsalways seesreactin the map. One variant answers{ path: args.path, external: true }, the other{ external: true }. Released Bun fails the first with the invalid output above and the second withCould not resolve.ResolveExternalSamePathandResolveExternalRewriteSurvivesBarrelRevisitpass on main. They pin cases that broke in earlier revisions of this PR: an answer that returns the specifier unchanged, and a rewritten external re-export in asideEffects: falsebarrel that a later consumer un-defers, next to a bundled module that shares its path text. #39874 and #41230 cover both today.History: this PR first carried the printed path fix, the entry point error, an
IS_EXTERNALflag, and aPendingImportenum. #39799 took the entry point error, #39874 made the flag unnecessary, #41230 took the path fix, and the enum only served a queue (resolve_tasks_waiting_for_import_source_index) that is not reached today, as #41230 notes. The branch was rebuilt on main with the two remaining fixes.Earlier revisions linked issue 2805 by mistake. That issue is about
defineandloadersinBun.build. No issue tracks either fix here.In this container,
bun-build-api.test.ts>bytecode: function record on an encoder page boundaryandbytecode: repeated builds don't retain the generated codeexceed their 5 s budget under the ASAN debug build with main's sources as well. They do not involve plugins.no test proof · iteration 8 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_plugin.test.ts