Repository navigation
Conversation
`has_bun_pragma` moved the cursor past a matched `@bun-cjs` or `@bytecode` and then stepped once more, so it never examined the byte after the keyword. When that byte was the `\n` that ends the pragma line, the scan read the next line too. A lone `@` before the `\n` did the same. The `@` arm now compares at `contents[cursor + 1..]` and leaves the cursor on the last byte of a matched keyword. The loop has one step, and it examines every byte of the pragma line.
The module loader gives a `// @bun` source to JSC as Latin-1, and there a `//` comment ends at `\r` or `\n`. `has_bun_pragma` stopped at `\n` only, so it read the code after a bare `\r` as pragma text. Tests: a bare CR row, a `bun -e` row, and a bare `@bytecode` token in the sidecar rows.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughChangesBun pragma parsing
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: stopping the // @bun keyword scan at the pragma line. |
| Description check | |
The description explains the problem, fix, scope, known limitations, reproduction, and verification results. It does not use the template headings exactly, but it provides the required change summary … |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description explains the problem, fix, scope, known limitations, reproduction, and verification results. It does not use the template headings exactly, but it provides the required change summary and detailed verification evidence.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Updated 12:05 AM PT - Oct 20th, 2026
❌ @robobun, your commit 65b1c85 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46122That installs a local version of the PR into your bun-46122 --bun |
|
Status: ready for review. The diff is green. The one red CI job is not from this change. Repro (main and Bun 1.4.3): printf '// @bun @bytecode\nconsole.log("the text @bun-cjs is on the second line");\n' > esm.js
bun esm.js
# prints the line, then:
# TypeError: Expected CommonJS module to have a function wrapper. If you weren't messing around with Bun's internals, this is a bug in Bun
# exit 1With this PR (#46122) the file runs as an ES module and exits 0. In CI (build 129232 on 65b1c85): 179 jobs passed, 1 failed, 2 did not run.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the removed cursor >= contents.len() guard against the new contents[cursor + 1..] slice in has_bun_pragma (src/js_parser/parse/parse_entry.rs:2581): the loop bound end is contents.len(), so cursor + 1 <= len holds on every iteration and a source ending in a bare @ cannot panic. I also traced the keyword arithmetic — a match adds the keyword length, leaving the cursor on the keyword's last byte, and the shared cursor += 1 lands on the following byte, so a \n directly after @ bun-cjs/@ bytecode now terminates the scan as intended.
Extended reasoning...
The change rewrites the @ arm of the // @ bun pragma keyword loop in src/js_parser/parse/parse_entry.rs and adds a \r line terminator, plus a new subprocess test matrix in test/bundler/transpiler/bun-pragma.test.ts; it touches the module loader's classification of already-bundled sources (ESM vs CJS, .jsc sidecar read) but no auth, crypto, or injection surface. Inline findings on glued-keyword behavior changes, the missing word-end rule, and the hashbang skip ignoring \r already signal that a human should weigh those design decisions, so this note only records the bounds and cursor-arithmetic checks that were ruled out.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/js_parser/parse/parse_entry.rs— Users with an existing already-bundled file whose pragma line holds a glued keyword now get a different module kind, or a.jscsidecar read, than on main. At parse_entry.rs:2581 the@arm now peeks at every@, so// @ bun @@ bun-cjsand@ bytecode@ bun-cjsbecome CommonJS and// @ bun @@ bytecodeand@ bun-cjs@ bytecodenow request bytecode; on main the byte after a keyword or lone@was skipped and these resolved differently. A.jscnext to such a file is now loaded. Fix: either keep main's resolution for these four spellings or add a word-end rule so a keyword only matches when followed by space, tab, CR, LF or EOF, and cover all four spellings in the test.Why this was flagged
If any stored bundle or hand-written loader file already carries
// @ bun @@ bytecodeor// @ bun @ bun-cjs@ bytecodeon its first line, the loader behaviour flips after this merge. On the base, at@@ bytecodethe first@advanced cursor onto the second@, saw it was notb, advanced again ontob, sobytecodewas never matched and no.jscwas opened. With the new arm at parse_entry.rs:2581-2588 the second@is visited andbytecodematches, so src/bundler/transpiler.rs:1630-1677 opens<file>.jscand hands it to JSC as bytecode for the source. Similarly@ bytecode@ bun-cjswas ESM on base and is now CommonJS. The only in-tree writer emits space-separated keywords, so no bun build output changes, but the pragma is a documented public format that other tools and users write; the PR tests only@@ bun-cjs. Remedy: add a word-end check after each keyword match so glued spellings keep one well-defined meaning, and test all four.Verification: Base: on
@the arm didcursor += 1; if contents[cursor] != b'b' { cursor += 1; continue; }, so the byte after a lone@was never examined. New code (src/js_parser/parse/parse_entry.rs:2578-2592):let slice = &contents[cursor + 1..];on every@, so a second@there is visited.@@ bytecode->Bytecode. When the result isBytecode/BytecodeCjs, src/bundler/transpiler.rs:1630-1680 opens<path>.jsc. -
🟣
src/js_parser/parse/parse_entry.rs— Users running a CR-only file with a hashbang above// @ bun @ bun-cjsget their module silently misloaded, even though this PR now says the pragma line ends at\r. The hashbang skip at parse_entry.rs:2555 still scans only for\n, so after#!/usr/bin/env bun\rit either runs off the end and returns None, or lands on a later line and misses the pragma. The lexer at src/js_parser/lexer.rs:1288 ends the hashbang at CR, LF, U+2028 and U+2029, so the two readers disagree. Fix: stop the hashbang skip at the same terminators the pragma line uses (\nor\r), in this PR, since it is the sibling site of the same line-ending class.Why this was flagged
The trigger is a source whose hashbang line ends in a bare
\r, e.g. the output ofbun fmt --end-of-line=cron abun build --target=bun --format=cjsbundle. It arrives through the runtime loader viahas_bun_pragmaat parse_entry.rs:888 with has_hashbang true, because the lexer at src/js_parser/lexer.rs:1288 ends the hashbang at 0x0D. The skip loop at parse_entry.rs:2555-2560 only compares against b'\n'; with no LF in the file it returns None, so the file is transpiled as plain JS and the CJS wrapper function expression is evaluated and discarded, printing nothing and exiting 0. With a stray LF later in the file, cursor lands on an arbitrary line, the prefix check at 2567 fails, and the same silent misload happens. The base has the same loop, but this PR adds\ras a pragma line terminator at parse_entry.rs:2577 and tests both 'a bare CR ends the pragma line' and 'below a hashbang line' separately, never together; REVIEW.md requires fixing every sibling site in the same class in the same PR. Remedy: terminate the hashbang skip at b'\n' | b'\r' as well.Verification: src/js_parser/parse/parse_entry.rs:2554-2565 (unchanged by the diff) only looks for
\n. The lexer (src/js_parser/lexer.rs:1288) ends the hashbang at CR as well, so for#!/usr/bin/env bun\r// @ bun @ bun-cjs\rthe loop returnsNoneand the pragma is missed. On the base commit the same input takes the same route, so merging makes nothing worse. The new test row at test/bundler/transpiler/bun-pragma.test.ts:61 only covers a hashbang terminated by\n.
The reader now examines every byte of the pragma line, so a keyword is read at every `@`. Add rows for `@@bytecode @bun-cjs`, `@bytecode@bun-cjs` and `@bun-cjs@bytecode` beside the `@@bun-cjs` row. Each fails on main.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the final state of this PR (including the test-only commit 3bfaecd) and found no bugs; because it changes which spellings of the // @ bun line classify a file as CommonJS or bytecode, a maintainer should still weigh in on that semantics choice.
What was reviewed:
- Diffed old vs new control flow of the
@arm inhas_bun_pragma(src/js_parser/parse/parse_entry.rs:2578-2588): thecontents[cursor + 1..]slice is in bounds because the loop guard givescursor < contents.len(), and after astarts_withmatch the+= lenthen+= 1cannot overshootend. - Traced the consumer in src/bundler/transpiler.rs:1630-1686: the
.jscsidecar read is gated onvirtual_source.is_none() && allow_bytecode_cache, so thebun -etest exercises only the ESM/CJS kind, as intended. - Checked the sidecar harness against existing suites:
vm.ScriptproduceCachedDatais implemented in NodeVMScript.cpp and the[Disk Cache] Cache hit for sourceCodeprobe underBUN_JSC_verboseDiskCache=1is the same signal test/bundler/bun-build-compile.test.ts already relies on.
Extended reasoning...
The change is a ten-line rewrite of the keyword loop in Parser::has_bun_pragma plus a new test matrix in test/bundler/transpiler/bun-pragma.test.ts; it touches no auth, crypto or injection surface, though the pragma does gate whether the loader opens a .jsc sidecar. The Rust change is small and its bounds arithmetic checks out, but it deliberately alters classification for glued spellings (@@ bun-cjs, @ bytecode@ bun-cjs, @ bun-cjs@ bytecode) and adds a bare-CR terminator, which the PR itself lists as a semantics decision. The sibling hashbang-skip loop still ends only at \n and the missing word-end rule (already flagged inline in the earlier run) remain explicitly out of scope per the description. The tests could not be executed in this environment, so the .jsc cache-hit control row rests on the author's claim.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Each arm of the keyword loop now steps by the length of what it matched: `@bun-cjs`, `@bytecode`, or one byte. This is the form of the prefix test above the loop, and it needs no shared step after the match.
|
Answers to the review points. The code changes are in 3bfaecd and 77dccac. A keyword directly after an A hashbang line that ends with a bare bun build --target=bun --format=cjs --banner $'#!/usr/bin/env bun\r// text' entry.js --outfile out.cjs
bun out.cjs
No word end after The comment in the The PR body lists the last two bugs under Downsides and "Not changed here". No issue tracks them yet. |
There was a problem hiding this comment.
I reviewed the latest push and didn't find any bugs. A human look is still worthwhile because the final rule ("a keyword is read at every @ of the pragma line, with no word-end check") deliberately changes the kind for the four glued spellings and leaves the word-end rule open, which is a loader-semantics decision rather than a pure bugfix.
What was reviewed:
- The rewritten loop in
src/js_parser/parse/parse_entry.rs: bounded bycursor < end, every arm breaks or advances,starts_withoncontents[cursor..]cannot overrun; the last commit is step-for-step equivalent to the previous push (7+1 == len("@ bun-cjs")). - The
.jscsidecar tests:BUN_JSC_verboseDiskCacheand the "Cache hit" line are already relied on bybun-build-compile.test.ts,vm.Script({ produceCachedData })is implemented inNodeVMScript.cpp, andtranspiler.rsonly opens<path>.jscfor theBytecode/BytecodeCjsvariants, so thereadsSidecartrue/false rows are load-bearing against the old scan. - Harness conventions:
using/await using, concurrent pipe drain, combinedtoEqualobject,tempDir,it.concurrent.each.
Extended reasoning...
The PR rewrites the ~15-line keyword scan in Parser::has_bun_pragma (src/js_parser/parse/parse_entry.rs) so the // @ bun line ends at \n or \r and each @ arm advances by exactly what it matched, and adds a test matrix in test/bundler/transpiler/bun-pragma.test.ts covering entry/require/import, bun -e, and .jsc sidecar reads. The security-relevant surface is the gate that decides whether a file is handed to JSC untranspiled and whether a sibling .jsc bytecode file is loaded; the change narrows that gate to the pragma line, which is strictly safer than the old scan that read into the next source line. No CODEOWNERS entry covers either file, the bug hunt ran dry, and index arithmetic is bounds-safe. Defer rather than approve because the PR intentionally changes the kind assigned to @@ bun-cjs-style glued spellings and explicitly leaves the word-end rule as a separate design decision, which a maintainer should weigh.
Problem
Parser::has_bun_pragma(src/js_parser/parse/parse_entry.rs:2574) reads the// @bunline for the module loader. It skips the byte after@bun-cjs,@bytecodeor a lone@, so it can read past the\n. A bare\rdoes not stop it.bun build --target=bun --format=cjsoutput with@bytecodeon line 2 makes the loader read<file>.jsc. JSC runs that file if it accepts it.// @bun @bytecode\nconsole.log("@bun-cjs");fails withTypeError: Expected CommonJS module to have a function wrapper. 1.3.5 runs it. fix(parser): fix bytecode CJS pragma detection after shebang #25868 (1.3.6) widened the scan.Fix
@bun-cjs,@bytecode, or one byte. The loop stops at\nor\r.has_bun_pragmafor a minified lodash bundle: 393,293 instructions to 69. No pragma: 26 to 22.test/bundler/transpiler/bun-pragma.test.ts. 18 of 19 new tests fail on main.Background
bun build --target=bunoutput starts with// @bun.@bun-cjsmarks a CommonJS wrapper function.@bytecodemakes the loader read<file>.jsc.//comment ends at\ror\n.strings::index_of_charto cut the line first: one more call per pragma source, same result.Downsides
bun buildnever writes change kind, as in 1.1.42:@@bun-cjs,@bytecode@bun-cjs,@@bytecode,@bun-cjs@bytecode.// @bun @foocosts 121 instructions, up from 96.// @bunnyreads as the pragma,@bun-cjsxas@bun-cjs.Notes
Repro
esm.js// @bun\rand the same code lineOn main a space before the
\n, or a CRLF line end, hides the bug. The skipped byte is then the space or the\r.No GitHub issue reports this. It was found by a read of the code.
History
self.lexer.endto the end of the text, so that the pragma is found below a hashbang line. Since then the scan reads the whole next line.Where it shows
bun build --target=bun --format=cjs --minifyof a file that holds the string"@bytecodealliance/preview2-shim"puts that string on line 2. On main the loader opensout.cjs.jscon each load. With this PR it does not. A--banner '// built by @bytecode-labs'does the same, because the banner follows the{of the wrapper on line 2..jscthat JSC accepts for the text, main logs[Disk Cache] Cache hit for sourceCodeand uses the bytecode.docs/bundler/bytecode.mdxnames the@bytecodepragma as the gate for that read.// @bun @bytecodeabove ES module code, or// @bun @.bun buildwrites the first only with--compile, and a compiled executable is not affected.Bun.build({ compile, bytecode: true, format: "esm" })returns that text asoutputs[0]. A saved copy fails on main withSyntaxError: Unexpected keyword 'export'when its first code line holds@bun-cjs.bun -e, stdin andblob:modules reach the same reader. On main,-eand stdin run the text as a CommonJS script, so animportstatement on line 2 is a SyntaxError. Ablob:import throws the TypeError.bun fmt --end-of-line=crturns// @bun\n...into// @bun\r.... The result fails on main.The rule after this PR
The pragma line ends at the first
\nor\r. A keyword is read at every@of that line. 1.1.42 read the four spellings below the same way.// @bun @@bun-cjs// @bun @bytecode@bun-cjs.jsc.jsc.jsc// @bun @@bytecode.jsc.jsc// @bun @bun-cjs@bytecode.jsc.jscThe test has a row for a keyword after a lone
@(@@bun-cjs,@@bytecode @bun-cjs) and after a keyword (@bytecode@bun-cjs,@bun-cjs@bytecode). A word-end rule would change the two glued rows.Numbers
Release builds on linux-x64: main at 3880151, and the same tree with this change. The instruction counts are gdb single steps from the entry of
has_bun_pragmato its return.// @bun, LF// @bun, CRLF@bytecode/@bun-cjs/ both, LF, short code line// @bun @foo// @bun @at the end of the text// @bun @bun-cjs, first code line of 4 / 8 / 16 KiBhas_bun_pragma: 201 for a graph of 201 modules (CommonJS and ES module), 104 forrequire("express"). Both builds give the same counts.has_bun_pragmagoes from 353 to 326 bytes. Its only call target stayspanic_bounds_check.sizeof the stripped binary gives 107,067,334 bytes of text for both builds.@@bun-cjsand@bun-cjs@bytecode. None of the 44 rows with a header thatbun buildwrites changes. No row moves between pragma and no pragma.<file>.jscper load of the--bannerbuild above: 1 to 0. The same build with--bytecode: 1 to 1.perf_event_open, interleaved runs, medians). Minified lodash bundle withBUN_JSC_useConcurrentJIT=0 BUN_JSC_useConcurrentGC=0: 39,013,027 to 38,606,348 (1.04% less, noise 0.02%). With default settings the noise is 2.1% and hides the change. A script with no pragma: 3,761,160 to 3,763,113 (noise 3,312).Not changed here
Each of these fails the same way on main and on this PR. 1.1.42 runs all of them.
// @bundleInto:commonabove TypeScript fails withSyntaxError: Unexpected token ':', because the file counts as already transpiled. A.jsfile below// @bunnyruns with its UTF-8 read as Latin-1 (// @bunpragma makes non-ASCII string literals decode as latin-1 at runtime #37161, Decode module text that comes from disk or bundler output as UTF-8 #38714).// @bun @bun-cjsxabove an ES module throws the wrapper TypeError.parse_entry.rs:2555) ends its line at\nonly.#!/usr/bin/env bun\rconsole.log("code");\n// @bun @bun-cjs\nprintscodeand throws the TypeError. A file with only\rline ends loses its pragma: the wrapper never runs, exit 0.bun buildcuts a hashbang banner at the first\n(postProcessJSChunk.rs:413), so--banner $'#!/usr/bin/env bun\r// text'writes#!/usr/bin/env bun\r// text\n// @bun @bun-cjs\n. The reader and the writer have to change together. transpiler cache: answer a hit before the parser and its heap are set up #43834 rewrites thatif.Related
// @bun @bun-cjsfile as the CommonJS module inside its wrapper #45726 call this reader and do not change the loop. Either merge order works.--bytecodewith--format=esmwithout--compile#29291 (draft) would letbun buildwrite// @bun @bytecodeabove ES module code.Tests
test/bundler/transpiler/bun-pragma.test.tshas 26 tests. Release build of main: 8 pass, 18 fail. Debug build of this PR: 26 pass.// @bun @bytecode @bun-cjsreads its.jsc.test/cli/run/commonjs-invalid.test.ts,test/cli/run/run-cjs.test.ts,test/bundler/transpiler/runtime-transpiler.test.ts,test/bundler/bundler_banner.test.ts,test/bundler/bundler_bun.test.ts,test/bundler/bundler_cjs.test.ts.Self-review, not done