bundler: give each module its own __dirname and __filename binding

[robobun]

Problem

• A bundle injects var __dirname = "<source dir>" into each module that uses it (same for __filename). Unwrapped modules share one scope, so every declaration prints as __dirname and the last one, the entry point, wins (__dirname handled incorrectly when building #17188).
• Cause: src/js_parser/p.rs:2743 declares both refs Kind::Unbound, and the renamer never renames an unbound symbol (src/js_printer/renamer.rs:540).

Fix

• When bundling, parse_entry.rs changes both refs to Kind::Hoisted after it has injected the declarations, so the renamer numbers them per module. Unbound only matters during the visit pass (define, visit_expr.rs:261), which is over. A module with a direct eval() keeps them unbound, so eval("__dirname") still resolves.
• The unbound refs also reserved the names in every chunk, which hid that a top-level class __dirname {} collides with the cjs wrapper parameter. compute_initial_reserved_names now reserves all five parameter names for cjs output.
• The values do not change. The review of fix: __dirname and __filename collisions #17222 said: "This collision is intentional and important for making things like .node addons bundle successfully ... we need to add an option". Addons are CJS, and a CJS-wrapped module already had its own value on main (only the spelling of its declaration can change). Unwrapped modules only ever got the entry point's directory. --define __dirname=... still gives one uniform value. @Jarred-Sumner, please confirm that no option is needed.
• Verified: test/bundler/bundler_edgecase.test.ts, 11 new tests, 10 fail on main. Other suites in the notes.

Background

• Kind::Unbound: a symbol that the file does not declare. Kind::Hoisted: a var binding.
• rename_symbols_in_chunk names a chunk's top-level symbols in one scope. Unbound names are reserved chunk-wide, used or not. A hoisted symbol gets its name, or the name plus a number.
• cjs output is the body of a function whose parameters are exports, require, module, __filename, __dirname. A var may repeat a parameter name, a class may not.

Fixes #17188

Notes

Output of the repro from #17188 (sub/b.ts exports a function that returns __dirname, index.ts imports it) with this change:

// sub/b.ts
var __dirname = "/tmp/p/sub";
function subDir() {
  return __dirname;
}

// index.ts
var __dirname2 = "/tmp/p";
console.log("index:", __dirname2);
console.log("sub:", subDir());

With --minify-identifiers the two declarations become var r = ... and var n = .... With require() of an ES module, or import() without splitting, the module goes into an __esm closure and the linker hoists the declaration out of it, so on main even a top-level export const dir = __dirname in such a module got the wrong value (DirnameFilenamePerModuleEsmWrapper). In --format=internal_bake_dev output each module is already in its own closure, so the rename there is cosmetic. --compile uses the same linker path (DirnameFilenamePerModuleCompile).

CJS-wrapped modules: the declaration inside a __commonJS closure keeps its value. Its name is still __dirname unless an unwrapped module earlier in the chunk took that name, then it is __dirname2 and so on, and under --minify-identifiers it is minified like the other locals. DirnameFilenamePerModule pins the closure shape with a regex.

cjs output: with the five names reserved, the injected declarations print as __dirname2, __dirname3, ... and no longer shadow the wrapper's own __dirname. The values are unchanged. One visible consequence: a module that only reaches __dirname through eval() now sees the wrapper's value (the directory of the bundle) instead of the build-time path of whichever module was printed last. DirnameFilenamePerModuleCJSFormat pins this with an output directory that differs from the source directory. require was already reserved through the unbound require symbol of every module. It is in the list so that the list is the parameter list. The pre-existing "Require" entry in EXTRAS is left alone.

Output that changes for a single module: only a module that declares its own top-level __dirname or __filename in esm output. On main the bundler's unused unbound symbol reserved the name and the user's binding printed as __dirname2 (the rename part of #15996). Now it keeps its name. DirnameFilenameUserExported pins the single-module shape and DirnameFilenamePerModuleUserDeclared pins it next to injected declarations. A module that does not declare its own prints what it printed before, because the first hoisted symbol gets the plain name (DirnameFilenamePerModule has a module that does not use the names and checks this). The const to var part of #15996 is unrelated and stays open.

Direct eval: bun does not pin module-scope names for direct eval today (the module scope pop is commented out in parse_entry.rs, #35955 adds it for CommonJS files), so keeping these two symbols unbound is a per-symbol stand-in for that rule. Probe: with the contains_direct_eval exclusion removed, DirnameFilenamePerModuleDirectEval fails, because eval("__dirname") in the ESM module and in the CJS module both returned the directory of module a, which took the plain name. With the exclusion the eval module keeps __dirname and the other modules get __dirname2 and __dirname3. Two eval modules in one unwrapped chunk still collide with each other, as on main.

Why the kind change is unconditional: in this block bundling declares every used ref, so each ref is either declared here or unused. A per-ref guard on the use flags was tried (9a6e8f0) and removed again (375e544), because both flags are always false at that point. A later change that makes a bundled ref refer to an outer binding instead (a wrapper parameter, as #29066 and #35470 explore) has to leave that ref unbound. #17222 fixed the same bug in the Zig parser with hashed names and was closed today in favor of this PR. #29066, #35651 and #35955 change the same block or the same symbols for other bugs and need a rebase over this.

declare_common_js_symbol (p.rs:4321) creates both refs in every file before the visit pass. Visit-pass uses of the unbound kind that stay intact: define substitution (visit_expr.rs:261), the "is not declared in this file" export check (visit_stmt.rs:197), and the side-effect analysis of a bare reference. The linker reads the kind in two places. compute_reserved_names_for_scope is the one this change is about. computeCrossChunkDependencies.rs:207 now sees a declared symbol, but it is declared and used in the same file, so no cross-chunk import is created (DirnameFilenamePerModuleSplitting).

Whether to inline a build-time path at all (#4216, #29066, #35470) is a separate question that this change does not touch. DirnameFilenameDefine pins that --define still replaces every reference with one value.

Unchanged paths: the kind change is gated on options.bundle. The runtime CommonJS wrapper parameters (WrapMode::BunCommonjs in p.rs), the runtime var __dirname = import.meta.dir injection and bun build --no-bundle (with and without --minify-identifiers --format=cjs) produce byte-identical output before and after. The non-bundle printer also calls compute_initial_reserved_names, but there the refs are still unbound and so were already reserved.

Most tests read the values inside functions that the entry point calls after every module has run, which is where unwrapped modules show the collision. The __esm test also reads them at the top level.

Suites run with the debug build: bundler_edgecase, bundler_cjs, bundler_cjs2esm, bundler_bun, bundler_minify, bundler_splitting, bundler_regressions, bundler_banner, bundler_footer, the bytecode and cjs tests of bundler_compile, esbuild/default, bun-build-api, the __dirname test in cli.test.ts, bake/dev/esm, test/js/node/module/node-module-module.test.js. All pass.

---

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_edgecase.test.ts

[robobun]

Status: ready for review

Reproduced on main with two files that both use __dirname (bun build --target=bun ./index.ts --outfile out.js && bun out.js prints the entry directory for both modules). 10 of the 11 new edgecase/DirnameFilename* tests in test/bundler/bundler_edgecase.test.ts fail on the released build and pass with this branch. DirnameFilenameUserClassCJSFormat also fails on the first revision of this branch (before the reserved names change) with SyntaxError: Cannot declare a class twice: '__dirname'.

Verification: bun bd test test/bundler/bundler_edgecase.test.ts (162 tests on the rebased branch, all pass), plus the bundler suites listed in the PR notes.

CI: the new tests pass on every lane in every build so far. Each build had one red test that this diff does not touch and that also fails on main: test/cli/install/bun-patch.test.ts (Windows aarch64), test/bake/deinitialization.test.ts (Windows x64), and on the rebased head test/js/bun/http/bun-server.test.ts (Windows x64, a heap-stats assertion, also red on main build 102154). All three were reported separately. The branch is rebased onto current main, which also cleared the binary size comparison against #39770.

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 5 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 55fa93c5-d2a1-4201-aaf2-0b761bd15232

📥 Commits

Reviewing files that changed from the base of the PR and between 46fdd0a and 3689618.

📒 Files selected for processing (3)

• src/js_parser/parse/parse_entry.rs
• src/js_printer/renamer.rs
• test/bundler/bundler_edgecase.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ed75c46e-5ac2-4130-8a97-c04a2790184d

📥 Commits

Reviewing files that changed from the base of the PR and between 0252b16 and 9a6e8f0.

📒 Files selected for processing (2)

• src/js_parser/parse/parse_entry.rs
• test/bundler/bundler_edgecase.test.ts

Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review.

---

Walkthrough

The parser now hoists bundled __dirname and __filename references per module when direct eval() is absent. Tests cover module graphs, output formats, minification, code splitting, direct eval(), user bindings, compilation, and define.

Changes

Bundled path binding resolution

Layer / File(s)	Summary
Conditional path binding hoisting src/js_parser/parse/parse_entry.rs	The parser hoists __dirname and __filename references before declaration generation when the module has no direct eval().
Per-module output validation test/bundler/bundler_edgecase.test.ts	Tests validate module-specific path values across mixed ESM/CJS graphs, CJS output, identifier minification, compilation, and code splitting.
Binding exception validation test/bundler/bundler_edgecase.test.ts	Tests validate direct-eval behavior, user-defined and exported bindings, and define replacements.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The change directly addresses issue #17188 by giving bundled modules distinct bindings and includes tests for the reported directory-resolution bug.
Out of Scope Changes check	✅ Passed	The implementation and added edge-case tests support the linked issue and stated objectives without introducing unrelated changes.
Title check	✅ Passed	The title clearly and concisely describes the main change: assigning each bundled module its own __dirname and __filename binding.
Description check	✅ Passed	The description explains the problem, fix, scope, behavior, and verification results, including extensive test coverage.

---

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

[claude]

LGTM — a targeted, well-tested fix for #17188.

Checked that dirname_ref/filename_ref are indeed created as Kind::Unbound (p.rs:2743-2746), that slot_namespace() maps Unbound to MustNotBeRenamed so assign_name skips it (renamer.rs:540), and that compute_reserved_names_for_scope reserves the name chunk-wide for unbound symbols (renamer.rs:1109). The kind flip runs after visit_tracer.end(), so the visit-pass consumers of Unbound (define substitution, export check) are unaffected.
Verified the contains_direct_eval guard reads the module scope, which is where the flag propagates. The 7 new tests are runtime-verified (not snapshot-only) and cover ESM/CJS, minify, direct-eval, user-declared shadowing, splitting, and --define.
The one downstream reader of the new kind (computeCrossChunkDependencies.rs:207) is covered by the splitting test — the symbol is declared and used in the same file so no spurious cross-chunk import is created.

Extended reasoning...

Overview

Six-line change in src/js_parser/parse/parse_entry.rs that flips the symbol kind of __dirname/__filename from Unbound to Hoisted after the visit pass completes, gated on options.bundle && !module_scope().contains_direct_eval. This lets the chunk renamer give each module's injected var __dirname = "..." a distinct binding instead of every unwrapped module colliding on the literal name __dirname. Seven new itBundled tests in bundler_edgecase.test.ts, all runtime-verified against JSON output.

Security risks

None. This is bundler symbol-naming logic; no untrusted-input parsing, no FFI, no memory management, no auth/crypto surface.

Level of scrutiny

Moderate — the bundler is production-critical, but the change is minimal and surgical. I traced every source reference in the PR description: the initial Unbound declaration at p.rs:2743-2746, the renamer's slot_namespace() == MustNotBeRenamed skip at renamer.rs:540-544, the reserved-name computation at renamer.rs:1109/1119, and the cross-chunk-dependency reader at computeCrossChunkDependencies.rs:207. All check out. The kind flip happens strictly after the visit pass ends (line 1115) and before the var injection, so the visit-pass semantics that depend on Unbound (define substitution, unbound-reference side-effect analysis) are preserved. The direct-eval exclusion is the same guard pattern used elsewhere in the parser and is tested.

Other factors

• The PR description is unusually thorough: it enumerates every downstream consumer of the changed kind, explains why each is unaffected or covered, and references the prior Zig-parser fix (#17222) whose review asked to keep build-time inlining — this approach does.
• Test coverage hits the variant matrix required by the review guidelines: ESM + CJS, --format=cjs, --minify-identifiers, direct eval() in ESM and CJS, user-declared __dirname, code splitting with a shared chunk, and --define. Tests read the values from functions called after all modules run (the point where the collision is observable), and normalize paths for Windows.
• The bug-hunting system found no issues.
• Single-module bundles produce byte-identical output (first Hoisted symbol takes the plain name), so existing snapshot tests are unaffected — the author ran bundler_cjs, bundler_minify, bundler_splitting, esbuild/default and others.

[robobun]

^{Updated 10:11 PM PT - Aug 20th, 2026}

❌ @robobun, your commit 3689618 has 1 failures in Build #102237 (All Failures):

• test/js/bun/http/bun-server.test.ts - code 1 on 🪟 2019 x64

---

🧪   To try this PR locally:

bunx bun-pr 39714

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

bun-39714 --bun

[claude]

LGTM — small, well-gated symbol-kind flip with thorough coverage.

What was reviewed:

• Verified the kind change happens after the visit pass, so define substitution and the unbound-identifier checks in visit_expr.rs/visit_stmt.rs still see Kind::Unbound.
• Confirmed the p.options.bundle gate leaves runtime transpilation and --no-bundle untouched, and the contains_direct_eval guard keeps eval("__dirname") working.
• Checked renamer.rs — Hoisted symbols go through assign_name / find_unused_name while Unbound reserved the name chunk-wide, which is exactly the collision being fixed.
• Tests exercise the full variant matrix (ESM/CJS mix, format=cjs, minify-identifiers, splitting, direct eval, user-declared, define) and pin that non-using modules don't reserve the plain names.

Extended reasoning...

Overview

An 8-line change in src/js_parser/parse/parse_entry.rs that, after the visit pass and only when options.bundle is set and the module scope has no direct eval, flips p.dirname_ref / p.filename_ref from Kind::Unbound to Kind::Hoisted. Seven new itBundled tests in test/bundler/bundler_edgecase.test.ts. Fixes #17188 where multiple unwrapped modules in one chunk each emit var __dirname = "..." and the last one wins.

Security risks

None. This only affects the identifier the bundler prints for its own injected var __dirname / var __filename declarations. No user input parsing, no filesystem or network paths.

Level of scrutiny

Moderate — the change is inside the parser/bundler symbol pipeline, but it's tiny, precisely gated, and placed after the visit pass so every visit-time consumer of the unbound kind is unchanged. I traced the two post-visit consumers of .kind the PR notes name (compute_reserved_names_for_scope in renamer.rs and computeCrossChunkDependencies): the first is the intended behavior change, and the second sees a symbol declared and used in the same file so no cross-chunk import is created (covered by DirnameFilenamePerModuleSplitting). The unconditional flip (not just when uses_dirname) is deliberate so an unused unbound symbol doesn't reserve the name for the chunk, and DirnameFilenamePerModule's onAfterBundle pins that.

Other factors

The comment-cop bot's feedback about the paragraph-long comment was addressed in d8adf24 (now one line, reasoning moved to the PR description and a test assertion). No CODEOWNERS on src/js_parser/. The PR description enumerates the visit-pass and linker read sites and the test suites run; six of the seven new tests fail on main per the author's verification. The direct-eval guard mirrors existing contains_direct_eval handling elsewhere in the parser.

[robobun]

Context from the earlier fix of this bug. #17222 fixed the same collision in the Zig parser in 2025 with a hashed name per module (__dirname_<hash>). That branch patches src/js_parser.zig, which no longer exists, so I am closing it in favor of this PR. Jarred-Sumner requested changes on #17222 with this review:

Thank you for this - the changes look like they work as expected, but I think we need to add an option to control the behavior of __dirname and __filename and similar.

This collision is intentional and important for making things like .node addons bundle successfully. The behavior of __dirname and __filename when bundled is ambiguous.

The same note applies to this PR. This PR keeps the inlined build-time values and gives each module its own declaration name, so the modules no longer read each other's directory. It does not add an option. The decision on this PR decides that question as well.

[claude]

I reviewed this PR and didn't find any bugs — the dead needs_binding guard from my earlier comment was dropped in 375e544, and the kind change is now the simple unconditional form. A maintainer look is still needed on the design question the description flags: the predecessor fix (#17222) was blocked on adding an option for __dirname/__filename behavior, and this PR takes the position that none is needed.

What was reviewed

• Traced the new Kind::Hoisted block: gated on options.bundle, runs after the visit pass so define substitution and the export-not-declared check are unaffected; the direct-eval exclusion keeps the literal name reachable from eval'd code.
• Confirmed uses_dirname/uses_filename are still read after the reset at 1211-1212 (lines 1720-1784), so the mut and the reset stay load-bearing.
• Checked the 9 new tests cover ESM/CJS mix, format: cjs, minify-identifiers, direct eval, user-declared bindings, --compile, splitting, and --define; assertions read the values from functions called after all modules run, so the collision is actually observable.

Extended reasoning...

Overview

The PR touches two files: an 8-line addition in src/js_parser/parse/parse_entry.rs that flips the __dirname/__filename symbol kind from Unbound to Hoisted after the visit pass when bundling (and no direct eval), plus 9 new itBundled tests in test/bundler/bundler_edgecase.test.ts. The effect is that the renamer numbers the per-module var __dirname = "..." declarations instead of letting them collide chunk-wide. Fixes #17188.

Security risks

None. This is a bundler output-shape change (symbol renaming). No parsing of untrusted input, no syscalls, no memory management.

Level of scrutiny

Medium-high. The Rust change is tiny and mechanically sound — the visit pass is over, and the two linker read sites (compute_reserved_names_for_scope, computeCrossChunkDependencies) are covered by the description and by DirnameFilenamePerModuleSplitting. But it changes user-visible bundler output and the predecessor fix (#17222) was explicitly blocked by a maintainer on "we need to add an option to control the behavior of __dirname and __filename" because "this collision is intentional and important for making things like .node addons bundle successfully". The author's counter-argument (addons are CJS-wrapped, so their output is byte-identical) is plausible and the description asks for maintainer confirmation. That confirmation is the gate, not the code.

Other factors

• My prior inline nit (dead needs_binding guard) was addressed in 375e544. I'd suggested also dropping the uses_dirname = false reset and the mut, but those are still read at lines 1720-1784 for the runtime ESM import.meta.dir injection path — the author correctly kept them.
• Test coverage is thorough for a bundler behavior change: the variant matrix (format, minify, splitting, compile, direct eval, user-declared, define) is covered, and 8 of 9 fail on main per the robobun verification.
• The single-module output change (user-declared __dirname keeps its name instead of becoming __dirname2, the rename half of #15996) is pinned by DirnameFilenameUserExported.

[claude]

I reviewed this PR and didn't find any bugs — the earlier dead-guard nit was addressed in 375e544, and the follow-up CJS wrapper-parameter reservation in fbca53a is covered by DirnameFilenameUserClassCJSFormat. Deferring to a maintainer because the PR explicitly asks for confirmation that no user-facing option is needed, and #17222's review of the same fix requested one for .node-addon bundling.

What was reviewed

• parse_entry.rs: kind change is now unconditional under bundle && !contains_direct_eval, sits directly after the injection block, gated off the runtime/no-bundle paths.
• renamer.rs: CJS_NAMES now covers all five wrapper parameters; require was already reserved via MinifyRenamer's $ path but not for NumberRenamer — the new test pins the class-name-collision case.
• Checked that compute_reserved_names_for_scope and cross-chunk dependency computation both tolerate the Unbound→Hoisted change (splitting test covers the latter).

Extended reasoning...

Overview

Two small Rust changes plus 9 new itBundled tests. parse_entry.rs flips __dirname/__filename refs from Kind::Unbound to Kind::Hoisted after the bundler injects their var declarations, so NumberRenamer numbers them per module instead of reserving one chunk-wide name. renamer.rs extends the Format::Cjs reserved-name set from [exports, module] to all five wrapper parameters, since the previously-unbound __dirname/__filename symbols no longer reserve those names by accident.

Security risks

None. Bundler symbol-renaming logic; no untrusted input parsing, no I/O, no auth/crypto.

Level of scrutiny

Medium-high. The mechanical change is small and well-tested (9 tests covering ESM/CJS, __esm wrappers, minify, splitting, compile, direct eval, user-declared, --define), and 8 of them fail on main. But it changes bundled-output semantics for __dirname/__filename, and the author explicitly flags an open design question: #17222's review of the same bug said "we need to add an option" because the collision was intentional for .node addons. This PR argues addons are CJS-wrapped and thus byte-identical, but that argument needs a maintainer to accept or reject — I can't rule on it.

Other factors

• My prior inline nit (dead needs_binding guard) was addressed in 375e544 and the author correctly kept the pre-existing uses_* = false resets that later code reads.
• The renamer.rs change adds require to CJS_NAMES alongside __dirname/__filename; the comment says a top-level class with a wrapper-parameter name would be a syntax error in CJS output, and DirnameFilenameUserClassCJSFormat guards it.
• Three other open PRs (#29066, #35651, #35955) touch the same block per the description — merge order is a maintainer call.
• Tests are hermetic, platform-normalized (replaceAll("\\\\", "/")), and read values through functions after all modules evaluate so the collision is actually observable.

[claude]

I reviewed this PR and didn't find any bugs — the earlier dead-guard nit was addressed in 375e544, and the two follow-up commits (fbca53a reserving the five cjs wrapper parameter names, 1071cab adding the DirnameFilenameUserClassCJSFormat test) look correct. A maintainer look is still needed because the PR explicitly asks one to confirm that no option is required, and #17222 (the earlier fix for this same bug) was blocked on exactly that question.

What was reviewed:

• The Kind::Hoisted flip in parse_entry.rs runs after the visit pass and is gated on options.bundle && !contains_direct_eval; traced that define substitution, the export-not-declared check, and computeCrossChunkDependencies are unaffected.
• CJS_NAMES growing to 5 entries: ensure_total_capacity uses cjs_names_len, so no over/under-reserve; require overlapping the existing unbound reservation is a no-op put.
• The 11 new itBundled tests cover esm/cjs/minify/splitting/compile/eval/user-declared/define; the rel() helper normalizes separators so Windows output matches.

Extended reasoning...

Overview

Two small source changes plus 11 bundler tests. src/js_parser/parse/parse_entry.rs flips p.dirname_ref/p.filename_ref from Kind::Unbound to Kind::Hoisted after the bundler has injected the var __dirname = ... declaration, so the chunk renamer numbers each module's declaration instead of letting them all collide on the literal name (#17188). src/js_printer/renamer.rs extends the cjs-format reserved-name list from [exports, module] to all five wrapper parameters, which was previously masked by every module's unbound __dirname/__filename reserving the names chunk-wide.

Security risks

None. This is a bundler symbol-renaming change; no untrusted input parsing, no I/O, no auth/crypto surface.

Level of scrutiny

Medium-high. The mechanical change is small and well-contained, but it alters bundler output for every multi-module bundle that references __dirname/__filename, and it intersects a design question a maintainer previously raised. The predecessor PR #17222 received changes-requested from Jarred-Sumner: "we need to add an option to control the behavior of __dirname and __filename ... This collision is intentional and important for making things like .node addons bundle successfully." This PR argues the option is unnecessary (addons are CJS-wrapped and already got their own value; only unwrapped ESM modules change, and they were only ever seeing the entry point's directory by accident), and the description explicitly asks the maintainer to confirm. That is a product/design decision, not a code-correctness question, so it should not be auto-approved.

Other factors

• My prior inline comment (dead needs_binding guard) was addressed in 375e544; the current diff iterates both refs unconditionally.
• fbca53a fixed a real secondary issue this PR uncovered: without the unbound refs reserving the names, a top-level class __dirname {} in cjs output would collide with the wrapper parameter and produce a syntax error. DirnameFilenameUserClassCJSFormat pins this.
• Test coverage is thorough for a bundler change of this size — the variant matrix (esm wrapper, cjs format, minify-identifiers, direct eval, user-declared, user-exported, compile, splitting, define) is covered, each test asserts actual runtime values via run.stdout, and the rel() helper handles Windows path separators. CI is green on all lanes per robobun.
• Three open PRs (#29066, #35651, #35955) touch the same block and will need rebasing, which the description already calls out.

[claude]

I reviewed the current revision (bf05a09) and found no bugs — my earlier dead-guard nit was addressed in 375e544/b3f57dc0. Deferring to a maintainer because the PR itself asks Jarred-Sumner to confirm that no __dirname/__filename option is needed (the design point that blocked #17222).

What was reviewed:

• The Kind::Unbound → Hoisted flip runs after the visit pass and after the injection block, so define substitution and the export-not-declared check are unaffected; gated on options.bundle so runtime transpile and --no-bundle paths are untouched.
• CJS_NAMES growing to 5 entries: capacity reservation still uses CJS_NAMES.len(), and require/__dirname/__filename overlap harmlessly with existing unbound reservations in the non-bundle printer.
• Cross-chunk dependency computation and the contains_direct_eval carve-out are each pinned by a dedicated test; the 11 new itBundled cases cover esm/cjs/minify/splitting/compile/define/user-declared without hardcoded ports or network.

Extended reasoning...

Overview

Two small Rust changes plus 11 bundler tests. parse_entry.rs adds a 6-line block that flips p.dirname_ref/p.filename_ref from Kind::Unbound to Kind::Hoisted when options.bundle && !module_scope().contains_direct_eval, so the chunk renamer numbers each module's injected var __dirname = ... separately instead of letting them all collide on the reserved name. renamer.rs extends CJS_NAMES from [exports, module] to the full CommonJS wrapper parameter list so a top-level class __dirname {} in --format=cjs output is renamed rather than emitted as a duplicate declaration of a function parameter.

Security risks

None. This is bundler symbol-naming logic operating on parser-owned refs; no untrusted input parsing, no allocation sizing from external data, no auth/crypto/permissions surface.

Level of scrutiny

Medium-high. The mechanical change is tiny and well-understood (I traced the two linker consumers of symbol.kind — compute_reserved_names_for_scope and computeCrossChunkDependencies — and both behave as the PR notes describe; the splitting test pins the latter). But the behavior change is user-visible bundler output: every bundled module that references __dirname/__filename now gets its own value instead of the entry point's. That is clearly correct per #17188, but the maintainer review on the earlier fix (#17222) explicitly said the collision was "intentional and important for making things like .node addons bundle successfully" and asked for an option. The author has laid out why they believe no option is needed (CJS-wrapped addons already had their own value on main; only unwrapped modules change) and directly asked Jarred-Sumner to confirm. That is exactly the kind of design sign-off a human should give.

Other factors

• My previous inline comment (dead needs_binding guard) was fully addressed; the current diff iterates both refs unconditionally.
• Test coverage is thorough: unwrapped ESM, __esm wrapper, cjs format (including the new class-collision guard), minify-identifiers, direct eval carve-out, user-declared/exported bindings, --compile, splitting, and --define. robobun reports 10/11 fail on the released build and all pass on this branch, with CI green on every lane.
• All prior bot feedback (comment-cop, my nit) is resolved; the only open item is the explicit maintainer question in the PR description.

[claude]

I reviewed this PR and didn't find any bugs; the earlier dead-guard nit was addressed in 375e544. Leaving this for a maintainer because the PR itself flags an open design question — whether per-module __dirname/__filename needs an option, which the #17222 review previously blocked on.

What was reviewed:

• The Kind::Hoisted flip in parse_entry.rs — placed after the injection block, gated on options.bundle && !contains_direct_eval, and applied to unused refs so they no longer reserve the name chunk-wide.
• compute_initial_reserved_names — the three added CJS wrapper params only affect Format::Cjs; the non-bundle printer path is unchanged because the refs are still unbound there.
• The 11 itBundled tests cover ESM/CJS, __esm wrapper, minify, splitting, compile, direct-eval, user-declared bindings, and --define; outputs are normalized via rel() so the assertions are platform-independent.

Extended reasoning...

Overview

The PR touches three files: an 8-line change in src/js_parser/parse/parse_entry.rs that flips the __dirname/__filename symbol kind from Unbound to Hoisted after the bundler injects their var declarations; a 9-line change in src/js_printer/renamer.rs that expands the CJS_NAMES reserved-name list from 2 to 5 entries (adding require, __filename, __dirname); and ~380 lines of new itBundled tests in test/bundler/bundler_edgecase.test.ts. The mechanism is minimal and well-placed: the kind change happens after the visit pass (where Unbound matters for define substitution and export checks) and before the linker/renamer (where Hoisted lets NumberRenamer suffix collisions). The contains_direct_eval guard preserves eval("__dirname"). The CJS reserved-name change is defensive — it prevents a top-level class __dirname {} from colliding with the wrapper parameter now that the unbound refs no longer reserve the name by accident.

Security risks

None. This is bundler symbol-renaming logic; no untrusted input parsing, no auth/crypto/permissions, no syscalls or FFI.

Level of scrutiny

The code change itself is small, targeted, and thoroughly tested (11 tests covering the variant matrix, 10 of which fail on main). However, the PR carries an explicit design question: the author asks Jarred-Sumner to confirm that no user-facing option is needed, because the prior fix for the same bug (#17222) was blocked with "we need to add an option to control the behavior of __dirname and __filename... This collision is intentional and important for making things like .node addons bundle successfully." The PR argues that CJS-wrapped modules (which addons are) already had per-module values on main and are unaffected in value, only in binding name — but that argument should be confirmed by the maintainer who raised the concern, not auto-approved.

Other factors

My previous inline comment (dead needs_binding guard) was addressed in 375e544 and the current diff iterates both refs unconditionally. All prior inline threads are resolved. The PR description enumerates the linker/visit-pass consumers of symbol.kind and pins each with a test (computeCrossChunkDependencies → DirnameFilenamePerModuleSplitting, define → DirnameFilenameDefine, etc.). CI passes on all lanes per the robobun status. The remaining blocker is purely the maintainer sign-off on the design question, which the author has already surfaced.

[robobun]

Re-verified on main at ae7b8f4: the same kind flip (Unbound to Hoisted for the injected __dirname / __filename decls) gives each lifted CJS module its own binding, and a deferred read inside a function returns its own module's path for --target=bun|node, with and without --minify. This branch conflicts with main and needs a rebase before it can merge.