Conversation
|
Updated 2:35 AM PT - Jun 29th, 2026
❌ Your commit
🧪 To try this PR locally: bunx bun-pr 30085That installs a local version of the PR into your bun-30085 --bun |
WalkthroughUpdates ImportMetaObject.main to perform platform-aware, character-by-character path comparison (treating '/' and '' as equivalent on Windows) with an early Bun.isMainThread guard, and adds a bundler regression test verifying ChangesImport.meta.main Windows Path Handling
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
ae21717 to
ed326bc
Compare
27258aa to
f872e72
Compare
Bun.main slashes in compiled binariesimport.meta.main compare
There was a problem hiding this comment.
LGTM — the revised approach (slash-tolerant compare in the builtin, leaving Bun.main untouched) cleanly resolves the earlier require.main/process.mainModule concern; the two inline nits are optional polish.
Extended reasoning...
Overview
This PR fixes #30084: import.meta.main was always false in Windows --compile standalone binaries because import.meta.path (backslashes, via WebKit URL::fileSystemPath()) never ===-matched Bun.main (forward slashes, the standalone virtual path / requireMap key). The first revision normalized Bun.main to backslashes in getMain (BunObject.zig), which I flagged as regressing require.main / process.mainModule for --compile --format=cjs on Windows. The author reworked the fix to live entirely in the import.meta.main JS builtin (src/js/builtins/ImportMetaObject.ts): keep the fast === path, and on Windows only, fall back to a same-length char-by-char compare that treats / and \\ as equivalent. Bun.main and import.meta.path are unchanged, so the CJS requireMap lookup keeps working. A new compile/ImportMetaMainOptionalChain bundler test exercises the runtime (non-folded) path.
Security risks
None. import.meta.main is an informational boolean, not a security boundary. The change adds a Windows-only string compare over two short runtime-internal paths; no untrusted input, no filesystem/network access, no auth/crypto surface.
Level of scrutiny
Low–medium. The builtin is ~20 lines, the logic is straightforward, and process.platform !== "win32" is inlined/DCE'd at builtin codegen time, so non-Windows builds reduce to exactly the pre-PR behavior (path === main after the isMainThread check). On Windows the fallback only runs after the === fast path misses, so existing matching cases are unaffected. No CODEOWNERS coverage for the touched files.
Other factors
- The earlier blocking concern (CJS
requireMapkey mismatch) is fully addressed by the new approach and the inline thread is resolved. - The two remaining inline findings are nits: (1)
main.lengthcan throw if a user explicitly setsBun.main = nullon Windows (unvalidated setter), where pre-PR===returnedfalse; (2) the builtin convention is to use the tamper-proof$charCodeAtintrinsic. Both require deliberate, unusual user action to surface and don't affect the fix's correctness for the reported bug, so they're non-blocking polish. - A regression test is included; the actual behavior change is validated by the Windows CI lane.
…of compare Address review nits on #30085: - Guard `Bun.main` (not just `this.path`) against non-string: `setMain` stores whatever JSValue the user passes, so `Bun.main = null` would make `main.length` throw on Windows. Pre-PR `===` just returned false there; match that behavior with `typeof main !== "string"` (and symmetric guard on `path`) before reading `.length`. - Use the private `$charCodeAt` intrinsic instead of public `.charCodeAt`, so user code overriding `String.prototype.charCodeAt` can't flip the compare. `src/js/CLAUDE.md` requires this for builtins; `$charCodeAt` is declared in `src/js/builtins.d.ts` and already used by node/url.ts, node/fs.ts for the same reason.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (builtin-only fix, typeof main guard, $charCodeAt).
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows --compile standalone executables when the check isn't constant-folded (e.g. import.meta?.main). The root cause is a separator mismatch: Bun.main is the forward-slash standalone virtual path (B:/~BUN/root/..., which must stay that way for the CJS requireMap key), while import.meta.path is backslash-normalized by WebKit's URL::fileSystemPath() on Windows. The fix is entirely inside the main() getter in src/js/builtins/ImportMetaObject.ts: after the fast-path ===, on Windows only, do a length-gated char-by-char compare that treats / (0x2F) and \\ (0x5C) as equivalent. A new compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts exercises the runtime path by using optional chaining to defeat the transpiler fold.
Security risks
None. import.meta.main is not a security boundary; the change only loosens an equality check on Windows to treat the two path-separator code points as equivalent, gated by equal length and string-typed operands. The compare now uses the tamper-proof $charCodeAt intrinsic rather than the public prototype method, so user prototype mutation can't influence it. No external input, no auth/crypto/permissions surface.
Level of scrutiny
Low–medium. The diff is ~30 lines in a single JS builtin plus one bundler test. On non-Windows builds, process.platform !== "win32" is inlined/DCE'd at builtin codegen time, so the post-fast-path code reduces to return false — i.e., behavior is byte-for-byte unchanged off Windows. On Windows the only behavioral expansion is accepting separator-only differences between two equal-length string paths. Bun.main, import.meta.path, require.main, and process.mainModule are all untouched.
Other factors
This is the third revision and all of my prior review points are now addressed and resolved: (1) the original getMain normalization that would have regressed require.main/process.mainModule for Windows --compile --format=cjs was replaced by this builtin-only compare; (2) the typeof main !== "string" guard was added so Bun.main = null keeps returning false instead of throwing; (3) $charCodeAt replaced public charCodeAt per the builtins convention (verified declared in src/js/builtins.d.ts and used in src/js/node/url.ts / fs.ts). No CODEOWNERS cover the touched paths. The bug-hunting system found no issues on this revision.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (builtin-local compare, typeof main guard, $charCodeAt).
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows --compile standalone executables (#30084). It modifies the main getter builtin in src/js/builtins/ImportMetaObject.ts to fall back to a char-by-char compare that treats / and \\ as equivalent on Windows, after the fast-path === miss. A regression test (compile/ImportMetaMainOptionalChain) is added to test/bundler/bundler_compile.test.ts that uses import.meta?.main to defeat the transpile-time fold and exercise the runtime builtin.
Security risks
None. This is a path-string equality check for a non-security-boundary value (import.meta.main is informational). The change is read-only, gated to Windows via process.platform (inlined/DCE'd at builtin codegen time on other platforms), uses the tamper-proof $charCodeAt intrinsic, and guards both operands with typeof ... !== "string" so it cannot throw on a user-overridden Bun.main.
Level of scrutiny
Low-to-medium. The builtin is small and the logic is mechanical (length check + per-char compare with two separator codepoints treated as equal). The original approach in this PR (normalizing Bun.main) would have regressed require.main/process.mainModule for --compile --format=cjs on Windows; the author switched to a builtin-local compare in response to that feedback, leaving Bun.main and import.meta.path unchanged for all other consumers. Two follow-up nits (guard main for non-string, use $charCodeAt) were also applied. All inline comments are resolved.
Other factors
No CODEOWNER covers the touched files. The fast === path is preserved, so non-Windows and non-standalone behavior is byte-identical to before. The new test passes on Linux/macOS as a no-op (both sides already forward-slash) and validates the fix on the Windows CI lane. The bug-hunting system found no issues on this revision.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (builtin-only compare, typeof main guard, $charCodeAt intrinsic), and the Windows-only fallback is dead-code-eliminated on other platforms.
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows --compile standalone executables, where import.meta.path (backslash, via WebKit's URL::fileSystemPath()) and Bun.main (forward-slash standalone virtual path) disagree on separators. The fix is contained to the main getter in src/js/builtins/ImportMetaObject.ts (~30 lines): after the existing fast-path ===, on Windows only, fall back to a length-gated char-by-char compare treating / and \\ as equivalent. A new compile/ImportMetaMainOptionalChain test in test/bundler/bundler_compile.test.ts exercises the runtime builtin (optional chaining defeats the transpiler's constant-fold).
Review history
I reviewed two earlier revisions:
- The first revision normalized
Bun.mainitself inBunObject.zig, which would have regressedrequire.main/process.mainModulefor Windows--compile --format=cjsbinaries. The author switched to the builtin-only compare in f872e72, leavingBun.mainand the CJSrequireMapkey untouched. - The second revision was missing a
typeof main !== "string"guard (soBun.main = nullwould throw on Windows) and used public.charCodeAtinstead of the tamper-proof$charCodeAtintrinsic. Both were fixed in 7f9a9f6, and I verified$charCodeAtis declared atsrc/js/builtins.d.ts:617.
All three inline comments are now resolved, and the current diff reflects all fixes.
Security risks
None. import.meta.main is not a security boundary; the only new method calls go through the JSC private intrinsic so user prototype tampering can't affect the compare. No filesystem, network, auth, or permission code is touched.
Level of scrutiny
Low–medium. The change is a small, well-scoped pure string comparison in a JS builtin. The process.platform !== "win32" check is inlined at builtin codegen time and dead-code-eliminated, so non-Windows builds reduce the fallback to return false — zero behavior change there. On Windows, non-standalone runs hit the fast === path (both sides already use backslashes) and return early; only the standalone-binary mismatch reaches the loop.
Other factors
No CODEOWNERS entry covers src/js/builtins. The bug-hunting system found no issues on this revision. The PR description thoroughly documents the root cause (with WebKit source references) and why the alternative approach was abandoned. The new bundler test passes on Linux/macOS today and will gate the Windows fix in CI.
There was a problem hiding this comment.
LGTM — all three review points are addressed (CJS requireMap left untouched, typeof main guard added, $charCodeAt intrinsic used), and non-Windows builds DCE the fallback to the original === behavior.
Extended reasoning...
Overview
Rewrites the import.meta.main builtin getter (src/js/builtins/ImportMetaObject.ts) so that on Windows, after the fast-path === miss, it does a char-by-char compare treating / (0x2F) and \ (0x5C) as equivalent. Adds a compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts that uses import.meta?.main to defeat the transpile-time fold and exercise the runtime builtin. Two files, ~45 net lines.
Review history
This PR went through two rounds of my feedback:
- Round 1 (🔴): the original approach normalized
Bun.mainto backslashes ingetMain, which would have regressedrequire.main/process.mainModulefor Windows--compile --format=cjsbinaries (those look up$requireMap.get(Bun.main)and the map is keyed by the forward-slash specifier). The author moved the fix entirely into the builtin compare, leavingBun.mainunchanged. - Round 2 (🟡 nits): missing
typeof main !== "string"guard (Bun.mainhas an unvalidated setter, soBun.main = nullwould have thrown onmain.length), and public.charCodeAtinstead of the tamper-proof$charCodeAtintrinsic. Both fixed in 7f9a9f6 — the current diff hastypeof path !== "string" || typeof main !== "string"guards and usespath.$charCodeAt(i)/main.$charCodeAt(i)(intrinsic confirmed atsrc/js/builtins.d.ts:617).
All inline comments are resolved; the bug hunting system found nothing new on the current revision.
Security risks
None. import.meta.main is an informational boolean, not a security boundary. The change adds no new tamperable surfaces relative to before ($charCodeAt is a JSC private intrinsic; .length on a string primitive is not prototype-tamperable). No filesystem, network, auth, or crypto code is touched.
Level of scrutiny
Low–medium. Per src/js/README.md, process.platform is inlined at builtin codegen time, so on non-Windows the entire fallback DCEs to return false and the emitted code is equivalent to the pre-PR return this.path === Bun.main && Bun.isMainThread. On Windows the only semantic change is relaxing the equality to treat / and \ as the same separator — a strict superset of the previous match set, and one that can only produce a false positive if two distinct modules have paths differing solely by separator direction (i.e., they are the same path).
Other factors
Neither file is covered by .github/CODEOWNERS. The new test follows the existing compile/ImportMetaMain pattern in the same file. The PR description accurately documents the root cause (WebKit URL::fileSystemPath() separator conversion vs. forward-slash standalone graph keys) and why the alternative fix was rejected. CI is running on the latest commit; the Linux lane is expected to pass trivially (both sides already forward-slash there) and Windows CI will validate the actual behavior change.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (CJS requireMap regression avoided, main type-guarded, $charCodeAt intrinsic used).
Extended reasoning...
Overview
Two-file change: src/js/builtins/ImportMetaObject.ts rewrites the import.meta.main getter to fall back to a slash-agnostic char-by-char compare on Windows after the === fast path, and test/bundler/bundler_compile.test.ts adds a compile/ImportMetaMainOptionalChain regression test that exercises the runtime builtin (optional chain defeats the transpiler fold).
Review history
This is my third pass. Round 1 flagged that the original approach (normalizing Bun.main to backslashes in getMain) would regress require.main/process.mainModule for Windows --compile --format=cjs binaries — the author moved the fix into the builtin compare instead, leaving Bun.main untouched. Round 2 flagged two nits: missing typeof main !== "string" guard (since Bun.main has an unvalidated setter) and use of public .charCodeAt instead of the tamper-proof $charCodeAt intrinsic. Both were addressed in 7f9a9f6, and all inline threads are resolved.
Security risks
None. import.meta.main is not a security boundary; the change only relaxes a string compare to treat / and \\ as equivalent on Windows. The $charCodeAt intrinsic prevents prototype-tampering from affecting the result, and the typeof guards prevent throws on overridden Bun.main.
Level of scrutiny
Low-to-moderate. The builtin is tiny and the diff is fully visible; process.platform !== "win32" is DCE'd at codegen time so non-Windows builds reduce to the pre-PR === || return false behavior. The Windows fallback only fires when === already failed and both sides are equal-length strings — false positives would require a path where / vs \\ is semantically meaningful in a non-separator position, which doesn't occur on Windows.
Other factors
Regression test added; CI retriggers in the commit log are for known unrelated flakes. No CODEOWNER-gated paths. The fix is now strictly additive on Windows and a no-op elsewhere.
2a7ab13 to
09c1f4c
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed (approach switched to builtin-only compare, typeof main guard added, $charCodeAt used).
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows standalone (--compile) executables. The root cause is a path-separator mismatch: Bun.main is the forward-slash standalone virtual path (B:/~BUN/root/...) while import.meta.path goes through WebKit's URL::fileSystemPath() which converts to backslashes on Windows. The fix changes the import.meta.main JS builtin (src/js/builtins/ImportMetaObject.ts) to, after the fast-path ===, do a Windows-only char-by-char compare treating / and \ as equivalent. A new compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts exercises the runtime builtin (optional chaining defeats the transpile-time fold).
Security risks
None. import.meta.main is not a security boundary. The change uses the tamper-proof $charCodeAt intrinsic per builtin conventions, and adds type guards so a user-set non-string Bun.main falls back to false rather than throwing. No auth, crypto, filesystem, or network surface is touched.
Level of scrutiny
Low-to-moderate. The change is ~25 lines in a single builtin getter plus one test. The Windows fallback is gated behind process.platform !== "win32" (inlined/DCE'd at builtin codegen time per src/js/README.md), so non-Windows builds reduce to the original === semantics. On Windows non-standalone runs, both sides already use backslashes so the fast-path === hits and the fallback never executes; the fallback only fires in the standalone case being fixed.
Other factors
This PR went through three revisions in response to my prior reviews, and all three concerns are now resolved (and marked resolved in the thread):
- The first revision normalized
Bun.mainitself, which would have regressedrequire.main/process.mainModulefor Windows--compile --format=cjsbinaries via therequireMapkey mismatch. The author switched to comparing inside the builtin instead, leavingBun.mainand the CJS path untouched. - The missing null/type guard on
main(which could throw ifBun.mainis set tonullvia its unvalidated setter) was fixed withtypeof main !== "string". - Public
.charCodeAtwas replaced with the private$charCodeAtintrinsic per builtin tamper-proofing convention.
The bug-hunting system found no issues on the current revision. The logic is straightforward and self-contained, the PR description is thorough, and there's a regression test.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (builtin-side compare instead of mutating Bun.main, typeof main !== "string" guard, and $charCodeAt intrinsic).
Extended reasoning...
Overview
This PR fixes #30084: import.meta.main always returned false in Windows --compile standalone executables when the check wasn't constant-folded by the transpiler (e.g. import.meta?.main). The root cause is a path-separator mismatch — Bun.main is the forward-slash standalone virtual path (B:/~BUN/root/...) while import.meta.path is produced via WebKit's URL::fileSystemPath() which backslash-normalizes on Windows. The fix is entirely inside the main getter builtin in src/js/builtins/ImportMetaObject.ts: keep the fast === path, and on Windows only, fall back to a char-by-char compare that treats / (0x2F) and \ (0x5C) as equivalent. A new compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts exercises the runtime builtin path.
Review history
This PR went through three rounds of feedback, all of which the author has addressed:
- The original revision normalized
Bun.mainingetMain(BunObject.zig), which would have regressedrequire.main/process.mainModulefor Windows--compile --format=cjsbinaries since those look up$requireMap.get(Bun.main)keyed by the forward-slash specifier. Author moved the fix into the builtin compare instead, leavingBun.mainuntouched. - Missing null/type guard on
main(sinceBun.mainhas an unvalidated setter). Author addedtypeof main !== "string"(and upgraded thepathguard totypeof path !== "string"for symmetry). - Public
.charCodeAtis prototype-tamperable. Author switched to the$charCodeAtprivate intrinsic per the builtins convention.
All three inline threads are resolved and the current diff reflects the fixes.
Security risks
None. import.meta.main is not a security boundary. The new code uses the tamper-proof $charCodeAt intrinsic, type-guards both inputs before dereferencing .length, and only adds behavior on the Windows code path (the process.platform !== "win32" branch is dead-code-eliminated at builtin codegen time on other platforms, so non-Windows builds reduce to the same fast === semantics as before). No new I/O, no auth/crypto, no untrusted-input parsing.
Level of scrutiny
Low-to-moderate. The builtin is small (~30 lines), the logic is a straightforward length-gated char compare with two ASCII separator codes, and the non-Windows hot path is unchanged. The earlier design concern (CJS requireMap key mismatch) was the only real risk and has been resolved by scoping the fix to the comparison rather than mutating Bun.main. No CODEOWNERS entry covers src/js/builtins/ImportMetaObject.ts or the bundler compile test.
Other factors
- Bug-hunting system found no issues on the current revision.
- The new test passes trivially on Linux/macOS (both sides already forward-slash) and validates the fix on Windows CI; the existing
compile/ImportMetaMaintest continues to cover the transpile-time fold path. - The
$charCodeAtintrinsic is declared atsrc/js/builtins.d.ts:617and used elsewhere (node/url.ts,node/fs.ts), so codegen support is established.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (approach moved to the builtin compare, typeof main guard added, $charCodeAt intrinsic used).
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows --compile standalone executables when the check isn't constant-folded by the transpiler (e.g. import.meta?.main). The fix is contained to the main getter in src/js/builtins/ImportMetaObject.ts (~30 lines): after the existing fast-path ===, on Windows it falls back to a length-gated char-by-char compare that treats / (0x2F) and \ (0x5C) as equivalent. A new compile/ImportMetaMainOptionalChain regression test is added to bundler_compile.test.ts.
Security risks
None. import.meta.main is a convenience boolean, not a security boundary. The change uses the tamper-proof $charCodeAt intrinsic and adds typeof guards on both operands, so the new code surface is no more user-influenceable than the old === compare.
Level of scrutiny
Low-to-moderate. The builtin is tiny and the diff is easy to reason about end-to-end. On non-Windows builds, process.platform !== "win32" is inlined and DCE'd at builtin codegen time (per src/js/README.md), so the post-fast-path code reduces to return false — i.e. non-Windows behavior is byte-identical to before. On Windows, the fallback only runs after === already failed, so the normal (non-standalone) bun run path where Bun.main and import.meta.path agree on backslashes still hits the fast path and is unchanged.
Other factors
This is my third pass on the PR. The first revision normalized Bun.main itself, which would have regressed require.main/process.mainModule for --compile --format=cjs on Windows; the author rewrote the approach to compare inside the builtin instead, leaving Bun.main (and the requireMap key) untouched. The second pass flagged two nits — guarding main against the unvalidated Bun.main setter, and using the $charCodeAt intrinsic — both now applied (and $charCodeAt is confirmed declared in src/js/builtins.d.ts and used in node/url.ts/node/fs.ts). All inline threads are resolved, the bug-hunting system found nothing on the current revision, and the new test locks in the runtime-builtin path. This is small, well-understood, and safe to approve.
There was a problem hiding this comment.
LGTM — both nits addressed ($charCodeAt intrinsic + typeof main guard), and the fix is now scoped to the builtin so require.main/process.mainModule are unaffected.
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows standalone (--compile) executables. The root cause is a path-separator mismatch: Bun.main is the forward-slash virtual path B:/~BUN/root/... (which must stay that way to match the CJS requireMap key), while import.meta.path comes through WebKit's URL::fileSystemPath() and gets backslashes. The fix adds a Windows-only fallback in the import.meta.main JS builtin that does a char-by-char compare treating / and \\ as equivalent. A new compile/ImportMetaMainOptionalChain bundler test exercises the runtime path (optional chaining defeats the transpiler's constant fold).
Security risks
None. import.meta.main is not a security boundary; the change only affects a boolean comparison result on Windows. The use of the $charCodeAt private intrinsic (rather than the public prototype method) was added in response to review feedback and prevents user prototype tampering from corrupting the compare.
Level of scrutiny
Low-to-medium. The change is ~25 lines in a single JS builtin getter plus one regression test. The non-Windows path is unchanged behaviorally (the process.platform !== "win32" guard is DCE'd at builtin codegen time, so non-Windows builds reduce to return false after the fast-path ===). The Windows fallback only runs when the strict-equality fast path misses, lengths match, and both values are strings — a narrow, well-guarded code path.
Other factors
This PR went through three review rounds:
- The initial approach (normalizing
Bun.mainingetMain) would have regressedrequire.main/process.mainModulefor--compile --format=cjson Windows; the author switched to the builtin-only approach, which leavesBun.mainand therequireMapkey untouched. - A missing
typeof main !== "string"guard (soBun.main = nullwouldn't throw) was added. - Public
.charCodeAtwas replaced with the tamper-proof$charCodeAtintrinsic per the builtins convention.
All three are now resolved in the current diff, all inline comments are marked resolved, and the bug-hunting system found no further issues. The $charCodeAt intrinsic is declared at src/js/builtins.d.ts:617 and process.platform branching in builtins follows the same pattern as NodeModuleObject.ts and shell.ts.
c0e4efe to
1bce08a
Compare
There was a problem hiding this comment.
LGTM — all earlier feedback addressed (builtin-only compare, typeof main guard, $charCodeAt); Windows-only fallback is DCE'd elsewhere.
Extended reasoning...
Overview
Replaces the one-line import.meta.main builtin (this.path === Bun.main && Bun.isMainThread) with a version that, on Windows only, falls back to a char-by-char compare treating / and \ as equivalent. Adds a compile/ImportMetaMainOptionalChain regression test that forces the runtime builtin (optional chain defeats the transpiler fold). Two files touched: src/js/builtins/ImportMetaObject.ts (~30 lines) and test/bundler/bundler_compile.test.ts (+18 lines).
Security risks
None. import.meta.main is not a security boundary. The new code uses the private $charCodeAt intrinsic so prototype tampering can't corrupt the compare, and it type-guards both operands before dereferencing. No external input, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The fast path (path === main) is unchanged, so non-Windows and non-standalone Windows runs behave exactly as before. Per the builtins codegen, process.platform !== "win32" is inlined and the entire fallback is dead-code-eliminated on Linux/macOS builds, so the diff is effectively a no-op there. On Windows the only behavioral change is that two paths differing solely in / vs \ now compare equal — which is the intended fix for #30084.
Other factors
This PR went through two revision rounds in response to my earlier comments: (1) the original Bun.main normalization was dropped to avoid regressing require.main/process.mainModule lookups in the CJS requireMap; (2) a typeof main !== "string" guard was added so Bun.main = null stays graceful; (3) .$charCodeAt replaced public .charCodeAt. All three inline threads are resolved, the bug-hunting pass on the current revision found nothing, and no CODEOWNERS rule covers these files. The added bundler test locks in the runtime path.
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the Windows-only slash-agnostic compare is self-contained and leaves Bun.main/requireMap untouched.
Extended reasoning...
Overview
This PR fixes #30084 by changing the import.meta.main JS builtin (src/js/builtins/ImportMetaObject.ts) to fall back to a char-by-char compare that treats / and \ as equivalent on Windows when the fast-path === against Bun.main misses. It also adds a compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts that exercises the runtime builtin (optional-chain defeats the transpiler fold).
Security risks
None. import.meta.main is not a security boundary; the change only relaxes a string equality check on Windows to be separator-agnostic. The builtin uses the tamper-proof $charCodeAt intrinsic per repo convention, and type-guards both operands so a user-overridden non-string Bun.main still returns false rather than throwing.
Level of scrutiny
Low-to-moderate. On non-Windows builds, process.platform !== "win32" is inlined and DCE'd at builtin codegen time, so the emitted code reduces to exactly the pre-PR semantics (Bun.isMainThread && this.path === Bun.main). On Windows, the fallback only runs after === fails and after both sides are confirmed strings of equal length. The earlier revision's regression risk (changing Bun.main and breaking require.main/process.mainModule via the CJS requireMap key) was eliminated by moving the fix entirely into the comparison — Bun.main and import.meta.path are unchanged.
Other factors
All three of my earlier inline comments (the requireMap regression, the missing typeof main !== "string" guard, and the public .charCodeAt vs. $charCodeAt) have been addressed and resolved in the current diff. The bug-hunting pass on this revision found nothing. Neither file is covered by CODEOWNERS. Subsequent commits since 7f9a9f6 are CI re-kicks only with no code changes.
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the slash-agnostic compare is scoped to the Windows builtin and leaves Bun.main / requireMap untouched.
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows --compile standalone executables when the check isn't constant-folded (e.g. import.meta?.main). It touches two files: the main getter in src/js/builtins/ImportMetaObject.ts (~30 lines, mostly comments) and a new compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts.
The fix replaces the single this.path === Bun.main && Bun.isMainThread expression with: an early isMainThread guard, the same fast-path ===, and — on Windows only — a char-by-char fallback that treats / (0x2F) and \ (0x5C) as equivalent. On non-Windows builds, process.platform !== "win32" is inlined at codegen time and the fallback DCEs to return false, so behavior is byte-identical to before.
Prior feedback
I raised three issues across two revisions, all now resolved and verified in the current diff:
- The original approach (normalizing
Bun.mainingetMain) regressedrequire.main/process.mainModulefor--compile --format=cjson Windows → reworked to compare inside the builtin instead, leavingBun.mainand therequireMapkey unchanged. - Missing guard for non-string
Bun.main(unvalidated setter) →typeof main !== "string"added. - Public
.charCodeAtwas prototype-tamperable → switched to the$charCodeAtintrinsic (confirmed declared atsrc/js/builtins.d.ts:617).
Security risks
None. import.meta.main is an informational boolean, not a security boundary. The change only relaxes a string compare on Windows for paths that already match modulo separator; it can't make a non-entry module's path equal an entry path that differs in any non-separator character. The $charCodeAt intrinsic prevents prototype-pollution-driven misbehavior. No filesystem, network, or auth surface is touched.
Level of scrutiny
Low–moderate. The builtin is small and the diff is easy to reason about line-by-line. The non-Windows path is provably unchanged after DCE. The Windows path is a bounded loop over two equal-length strings with a trivial equivalence class. Neither file has a CODEOWNERS entry.
Other factors
The new bundler test exercises the runtime builtin (optional chain defeats the transpile-time fold) and locks in the behavior. Recent commits on the branch are CI retriggers for known infra flakes, not code changes. CodeRabbit had no actionable findings, and the current bug-hunting pass found nothing.
There was a problem hiding this comment.
LGTM — both nits from my last pass are addressed (typeof main !== "string" guard and $charCodeAt intrinsic), and the slash-agnostic fallback is Windows-only via DCE so non-Windows behavior is byte-for-byte unchanged.
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows --compile standalone executables when the check isn't constant-folded (e.g. import.meta?.main). Two files touched: the main getter builtin in src/js/builtins/ImportMetaObject.ts (~30 lines, replacing a one-line === with a fast-path === plus a Windows-only char-by-char compare that treats / and \ as equivalent), and a new compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts.
Review history
This PR went through two rounds of my review. The first revision normalized Bun.main itself, which would have regressed require.main/process.mainModule for Windows CJS standalone binaries — the author switched to comparing inside the builtin instead, leaving Bun.main and the requireMap key untouched. The second round flagged two nits (guard main against the unvalidated Bun.main setter; use the tamper-proof $charCodeAt intrinsic), both now addressed in the current diff. All inline comments are resolved.
Security risks
None. import.meta.main is an informational boolean, not a security boundary. The change adds no new attack surface — it actually hardens the comparison by switching to the private $charCodeAt intrinsic so user prototype tampering can't affect it. No auth, crypto, filesystem, or network code is touched.
Level of scrutiny
Low-to-moderate. The builtin is small and the diff is easy to reason about end-to-end. process.platform !== "win32" is inlined and dead-code-eliminated at builtin codegen time (per src/js/README.md), so on Linux/macOS the fallback compiles away to return false and the getter is semantically identical to the pre-PR this.path === Bun.main && Bun.isMainThread. The Windows fallback only widens equality for the case where two same-length strings differ solely in / vs \ at matching positions — which on Windows are the same path. False positives would require a non-separator character to collide with a separator, which the explicit (a === 47 || a === 92) && (b === 47 || b === 92) check rules out.
Other factors
No CODEOWNERS entry covers src/js/builtins/. The PR description has a thorough root-cause analysis tracing both sides of the mismatch (StandaloneModuleGraph forward-slash keys vs WebKit URL::fileSystemPath() backslash conversion). The new bundler test exercises the runtime builtin path by using optional chaining to defeat the transpile-time fold. The Buildkite failure noted by robobun is a mass agent-expiration CI infra issue (matching the "ci: retrigger" commits at HEAD), not a test failure from this change.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (typeof main guard, $charCodeAt intrinsic, no Bun.main mutation).
Extended reasoning...
Overview
This PR fixes import.meta.main returning false in Windows --compile standalone executables when evaluated at runtime (e.g. via import.meta?.main, which the transpiler doesn't constant-fold). The fix is contained in the main getter builtin in src/js/builtins/ImportMetaObject.ts: after the existing fast-path ===, on Windows it falls back to a char-by-char compare that treats / (0x2F) and \ (0x5C) as equivalent. A regression test compile/ImportMetaMainOptionalChain is added in test/bundler/bundler_compile.test.ts.
I previously flagged three issues on earlier revisions, all now resolved in the current diff:
- The first revision normalized
Bun.mainitself, which would have regressedrequire.main/process.mainModulefor--compile --format=cjson Windows. Replaced with a compare-only approach that leavesBun.mainuntouched. - Missing guard for non-string
Bun.main(settable to any JSValue via the unvalidatedsetMain). Now guarded withtypeof main !== "string". - Public
.charCodeAtwas tamperable viaString.prototype. Now uses the private$charCodeAtintrinsic (declared atsrc/js/builtins.d.ts:617).
Security risks
None. import.meta.main is an informational boolean, not a security boundary. The Windows fallback only widens what counts as a match by treating the two path separators as equivalent — standard Windows path semantics — and only after length and type checks. No filesystem access, no privilege change, no user-controlled input beyond what was already compared by ===.
Level of scrutiny
Low-to-moderate. process.platform !== "win32" is inlined and dead-code-eliminated by the builtins codegen, so on Linux/macOS the new code reduces to the original === followed by return false — zero behavior change there. On Windows the fallback is a straightforward equal-length char loop. No CODEOWNERS cover these files (*.d.ts rule doesn't match .ts).
Other factors
The CI failure noted in the robobun comment is infrastructure-related (the branch's recent commits are all ci: rekick/ci: retrigger for agent expiration and Windows tempdir EPERM in the runner), not a test failure caused by this change. The bug hunting system found no issues on the current revision.
There was a problem hiding this comment.
LGTM — all prior feedback addressed (builtin-side compare instead of mutating Bun.main, typeof main guard, $charCodeAt intrinsic).
Extended reasoning...
Overview
This PR fixes #30084: import.meta.main was always false in Windows --compile standalone executables when evaluated at runtime (e.g. via import.meta?.main, which the transpiler doesn't constant-fold). The root cause is a path-separator mismatch between import.meta.path (backslashes via WebKit's URL::fileSystemPath()) and Bun.main (forward slashes, matching the CJS requireMap key). The fix rewrites the main getter in src/js/builtins/ImportMetaObject.ts to fall back to a char-by-char compare on Windows that treats / and \\ as equivalent, and adds a regression test in test/bundler/bundler_compile.test.ts.
Review history
I raised three issues on earlier revisions, all now resolved:
- 🔴 The original approach normalized
Bun.mainto backslashes, which would have regressedrequire.main/process.mainModulelookups in the CJSrequireMap. The author switched to comparing inside the builtin instead, leavingBun.mainuntouched. - 🟡 Missing
typeof main !== "string"guard (sinceBun.mainhas an unvalidated setter) — added. - 🟡 Use of public
.charCodeAtinstead of the tamper-proof$charCodeAtintrinsic — switched.
The current diff reflects all three fixes, and the bug-hunting system found no new issues on this revision.
Security risks
None. import.meta.main is not a security boundary — it's an informational boolean. The change uses the $charCodeAt private intrinsic so prototype pollution can't corrupt the compare, and the typeof guards prevent throws on user-overridden Bun.main. No new attack surface is introduced.
Level of scrutiny
Low–medium. The change is ~30 lines in a single JS builtin getter plus one bundler test. On non-Windows builds, process.platform !== "win32" is DCE'd at builtin-codegen time, so the function reduces to the semantic equivalent of the original this.path === Bun.main && Bun.isMainThread — zero behavior change off Windows. On Windows, the fast-path === still handles the non-standalone case identically; only the previously-broken standalone case takes the new fallback.
Other factors
The robobun CI comment shows build-zig failures on a few lanes for the latest build, but this revision touches no Zig code (only a TS builtin and a test file), and the branch's recent commits are all "ci: rekick/retrigger" for unrelated infrastructure flakes (Windows tempdir EPERM, expired agents). Those failures are not attributable to this change.
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the Windows-only slash-insensitive compare is now scoped to the builtin with proper type guards and $charCodeAt.
Extended reasoning...
Overview
Fixes import.meta.main returning false in Windows --compile standalone executables when the check isn't constant-folded (e.g. import.meta?.main). Touches two files: the import.meta.main builtin getter in src/js/builtins/ImportMetaObject.ts (~30 lines, replacing a one-line === with a fast-path === plus a Windows-only char-by-char compare that treats / and \\ as equivalent), and a new regression test in test/bundler/bundler_compile.test.ts.
Review history
This PR went through three rounds of my feedback, all now addressed and marked resolved:
- The original revision normalized
Bun.mainto backslashes ingetMain, which would have regressedrequire.main/process.mainModulefor--compile --format=cjson Windows (those look uprequireMapby the forward-slash key). Author moved the fix entirely into the builtin compare soBun.mainis unchanged. - Missing type guard on
main(sinceBun.mainhas an unvalidated setter) → author addedtypeof main !== "string". - Public
.charCodeAt(prototype-tamperable) → author switched to the$charCodeAtprivate intrinsic per builtin conventions.
Security risks
None. import.meta.main is an informational boolean, not a security boundary. The change introduces no new I/O, no parsing of untrusted input, and uses tamper-proof intrinsics.
Level of scrutiny
Low-to-moderate. The non-Windows path is byte-identical to pre-PR behavior after DCE (process.platform !== "win32" is inlined at builtin codegen time, so the whole fallback compiles to return false). The Windows path only relaxes the compare to treat / ↔ \\ as equal — both are reserved path-separator characters on Windows, so no real filename can contain them as literals, and the standalone virtual paths are bundler-controlled. False positives would require two distinct same-length paths that differ only in separator positions, which on Windows means they refer to the same file.
Other factors
Small, self-contained, well-commented, with a targeted regression test that defeats the transpiler fold to exercise the runtime builtin. No outstanding reviewer comments. Bug hunter found nothing in the current revision.
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the fix is now localized to the import.meta.main builtin so Bun.main (and the CJS requireMap lookups) stay untouched.
Extended reasoning...
Overview
Two files: the import.meta.main getter in src/js/builtins/ImportMetaObject.ts (~30 lines, replacing a one-line === compare with a fast-path === plus a Windows-only slash-agnostic fallback), and a new compile/ImportMetaMainOptionalChain regression test in test/bundler/bundler_compile.test.ts.
Security risks
None. import.meta.main is informational, not a security boundary. The fallback now uses the tamper-proof $charCodeAt intrinsic and guards both operands with typeof === "string", so user prototype tampering and Bun.main = null are handled at least as gracefully as the pre-PR code.
Level of scrutiny
Low–medium. On non-Windows builds, process.platform !== "win32" is inlined and DCE'd by the builtins codegen, so the function reduces to exactly the pre-PR semantics (isMainThread && path === main). The behavior change is confined to Windows, where it strictly widens a false result to true only when the two strings differ solely in / vs \\ — which is precisely the standalone-binary case from #30084.
Other factors
This PR went through three review rounds. The first revision normalized Bun.main and would have regressed require.main/process.mainModule for --compile --format=cjs on Windows; the author switched to the builtin-local approach in response. Two follow-up nits (guarding non-string Bun.main, using $charCodeAt) were both applied in 7f9a9f6, and all inline threads are resolved. No CODEOWNERS path matches. The new test exercises the runtime builtin (via import.meta?.main, which defeats the transpile-time fold) and locks the fix in for Windows CI.
|
This is a slop approach, the real fix is deeper and elsewhere. |
import.meta.main compareimport.meta?.main through the optional chain
b4ab9b0 to
0ed52c4
Compare
b319c44 to
1f1ac14
Compare
|
Heads up: the CodeRabbit walkthrough above is describing a stale diff. It mentions The current diff (HEAD
|
`bun build --compile` constant-folds `import.meta.main` to a boolean at build time, but the `e_dot` visitor skipped that rewrite whenever an optional chain was set. So `import.meta?.main` kept the runtime builtin, which compares `import.meta.path === Bun.main`. On Windows standalone binaries those disagree on path separator (`B:/~BUN/...` vs `B:\~BUN\...`), making the check always false (#30084). `import.meta` is never nullish, so the optional chain is a no-op. Allow the property-access rewrite through it for `e_import_meta` targets so the optional form folds the same way the plain form already does. Fixes #30084.
1f1ac14 to
dca1ccc
Compare
There was a problem hiding this comment.
Thanks — the trimmed comments look good and I have no remaining technical concerns with this revision; leaving final sign-off to a maintainer since this touches the transpiler visitor and the earlier approach drew an explicit objection that hasn't been revisited.
Extended reasoning...
Overview
The current diff is minimal: a 4-line change in src/js_parser/visit/visit_expr.rs that lets maybe_rewrite_property_access run when the e_dot target is EImportMeta even if optional_chain is set, plus a 22-line regression test in test/bundler/bundler_compile.test.ts. The reasoning is sound — import.meta is spec-guaranteed to be an object, so ?. on it is a semantic no-op and allowing the existing constant-fold through is safe. The test mirrors the established compile/ImportMetaMain test pattern (observing the fold via (() => …).toString()) and additionally checks a non-entry module folds to false. All of my prior feedback on this PR (the Bun.main regression, the .zig vs .rs mix-up, the comment-length nit) has been addressed; the latest push trims the comments per the repo's CLAUDE.md conventions.
Security risks
None identified. The change only widens a compile-time constant-fold to one additional syntactic spelling of an expression that was already being folded. No runtime APIs, path handling, or auth/permission surfaces are touched in this revision (earlier revisions that touched Bun.main / ImportMetaObject.ts were dropped).
Level of scrutiny
Medium-high. The transpiler visitor is a hot, correctness-critical path — a wrong rewrite here silently changes program semantics for every bundled file. The change itself is narrow and well-justified, but this PR has been through three distinct approaches (normalize Bun.main → runtime path compare in the builtin → transpiler fold), and a maintainer (alii) explicitly called the second approach out as "slop" with "the real fix is deeper and elsewhere" on May 4. The current transpiler-level approach is plausibly that deeper fix, but alii has not returned to confirm it satisfies the concern.
Other factors
- The pre-existing
.e_import_meta_mainprinter-precedence issue I flagged on May 5 is acknowledged as orthogonal and tracked separately; it's not a blocker here. - The new test directly asserts the fold happened (via
.toString().includes) rather than just observing runtime behavior, so it would catch a regression in either direction. - Given the maintainer's prior objection and the critical nature of the transpiler, I'm deferring rather than approving so a human can confirm the final approach is the one they want.
|
For a maintainer picking this up: the approach here is the transpiler-level fix, not the earlier builtin change @alii flagged as "slop, the real fix is deeper and elsewhere." That earlier revision (a slash-tolerant path compare inside the
No runtime builtin, no |
|
Stale PR review: keep open, rework. The bug in #30084 is real, but this diff does not fix the binary that the issue tells users to run. The repro repo builds Checked on Windows x64 with 1.4.3-canary (fc297d4):
So this PR fixes one spelling in one build path. The separator mismatch is the cause. That is the deeper fix that the comment from 2026-05-04 asked for. Wanted shape:
|
Fixes #30084.
Reproduction
Root cause
bun build --compilealready constant-foldsimport.meta.maintotrue/falsefor every module at build time. But thee_dotvisitor skipped that rewrite whenever an optional chain was set (x?.y), a reasonable default becausex?.yshort-circuits whenxis nullish.import.metais never nullish, so the optional chain onimport.meta?.mainis a no-op. Bailing out of the rewrite left the runtime builtin in the compiled binary, which comparesimport.meta.path === Bun.main. On Windows standalone executables those two disagree on path separator:Bun.mainis the graph entry nameB:/~BUN/root/entry.ts(forward slashes). It has to stay that way: the CommonJSrequireMapis keyed by the same specifier, andrequire.main/process.mainModuleboth look up withrequireMap.get(Bun.main).import.meta.pathis produced via WebKit'sURL::fileSystemPath(), which on Windows converts every/in the URL path to\(vendor/WebKit/Source/WTF/wtf/URL.cpp:239). Result:"B:\\~BUN\\root\\entry.ts".So
===always returnedfalseandimport.meta?.mainwas always falsy in Windows--compiled binaries. Linux/macOS were not affected because their standalone base path is/$bunfs/andfileSystemPath()doesn't touch separators off Windows.Fix
In the
e_dotvisitor (src/js_parser/visit/visit_expr.rs), allowmaybe_rewrite_property_accessto run through the optional chain when the target isEImportMeta. Sinceimport.metais guaranteed non-null, the short-circuit never fires and eliding it is a pure semantic no-op.import.meta?.mainnow folds totruein the entry module andfalseelsewhere at build time, exactly likeimport.meta.mainalready did.The runtime builtin and the standalone-graph separator choice are untouched. No other APIs change.
Verification
compile/ImportMetaMainOptionalChainintest/bundler/bundler_compile.test.tscaptures(() => import.meta?.main).toString()in both the entry and a non-entry module and asserts the fold left the literaltrue/false. Fail-before / pass-after confirmed locally:false\nfalse\ntrue\nfalsetrue\ntrue\nfalse\ntrue