cli: add --check flag to syntax-check a file without executing - #30981
cli: add --check flag to syntax-check a file without executing#30981robobun wants to merge 2 commits into
--check flag to syntax-check a file without executing#30981Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a ChangesSyntax check flag implementation
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/cli/Arguments.rs`:
- Around line 1134-1141: The conflict check incorrectly tests
!ctx.runtime_options.eval.script.is_empty() which misses cases like --eval "" or
--print ""; update the condition that follows ctx.runtime_options.syntax_check
to detect flag presence instead of script content by checking
args.flag(b"--eval") || args.flag(b"--print") (or any equivalent runtime
presence indicator), and keep the existing error path (Output::err_generic and
Global::exit(9)) so that any use of --check together with either --eval or
--print triggers the conflict error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 12987e53-0833-49e6-a0c4-23f1647a1ae1
📒 Files selected for processing (7)
completions/bun.zshsrc/jsc/bindings/wtf-bindings.cppsrc/options_types/context.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/mod.rssrc/runtime/cli/run_command.rstest/cli/run/check.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/check.test.ts`:
- Around line 161-174: Refactor the test "rejects combining --check with --eval
or --print" to use Jest's parameterized API (test.each) instead of a for-loop:
create a test.each([...]) that enumerates the flag variants
["-e","--eval","-p","--print"] and for each case spawn Bun with cmd [bunExe(),
"--check", flag, "1 + 1"], capture stdout/stderr/exit and assert the same three
expectations (stderr contains "either --check or --eval", stdout is empty,
exitCode !== 0); keep the existing Bun.spawn usage and variable names (proc,
bunExe, bunEnv) and preserve async handling so failures report per-case in test
output.
- Around line 58-61: The test currently asserts exitCode before guarding stderr
and verifying filesystem state; move the filesystem assertion using
Bun.file(`${dir}/ran.txt`).exists() to run before the exitCode assertion, insert
the guard pattern if (exitCode !== 0) { expect(stderr).toBe(""); } immediately
before asserting exitCode, and finally assert expect(exitCode).toBe(0);—keep the
existing stdout and stderr assertions (and the new guard) above the final
exitCode check so exitCode is asserted last.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 19364783-2846-4b8a-8206-2100edd0d18f
📒 Files selected for processing (2)
completions/bun.zshtest/cli/run/check.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/check.test.ts`:
- Around line 161-172: The test in test.each([...]) currently asserts any
non-zero exit code for the conflicting "--check" + eval/print flags; change the
final assertion from expect(exitCode).not.toBe(0) to expect(exitCode).toBe(9) so
the test verifies the exact contract for the conflict exit code; update the
assertion in the async test body that awaits proc.exited (the exitCode variable)
to assert equality with 9 while leaving the existing stdout/stderr checks and
Bun.spawn/using setup intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: eeb6082a-974e-4c59-9310-04be509dc2be
📒 Files selected for processing (1)
test/cli/run/check.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/cli/run/check.test.ts`:
- Line 20: The test currently asserts raw stderr is empty via
expect(stderr).toBe(""), which is flaky because ASAN can emit a one-time startup
warning; before the assertion, normalize stderr by splitting into lines and
filtering out any lines that start with "WARNING: ASAN interferes" (e.g., use
stderr.split(...).filter(line => !line.startsWith("WARNING: ASAN
interferes")).join(...)), then assert the filtered stderr is empty; apply the
same change for the other assertions in this file that use
expect(stderr).toBe("") (the occurrences around the other checks noted).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae402ef5-cabf-425c-b006-5bd31e4c885f
📒 Files selected for processing (1)
test/cli/run/check.test.ts
There was a problem hiding this comment.
All my earlier nits have been addressed or reasonably deferred and I didn't find anything new, but since this adds a new user-facing CLI flag wired into the core bun/bun run/node-mode dispatch (plus a drive-by wtf-bindings.cpp fix), it's worth a maintainer sign-off.
Extended reasoning...
Overview
Adds bun --check <file> (Node's --check/-c equivalent): a new syntax_check field on RuntimeOptions, a --check entry in RUNTIME_PARAMS_, an ~80-line RunCommand::exec_check that reads the file/stdin and runs bun_js_parser::Parser directly without booting the VM, early-exit dispatch hooks in exec_auto_or_run and exec_as_if_node, a 200-line test suite, a zsh completion entry, and an unrelated one-line assert → ASSERT fix in wtf-bindings.cpp.
Security risks
None identified. The new path only reads a user-specified file (or stdin) and parses it; it never executes code, resolves imports, or touches the network. The unsafe { ctx.log() } blocks follow the existing documented pattern for the process-lifetime CLI log.
Level of scrutiny
Medium. This is a new user-facing feature threaded through the core CLI dispatch chain — an area where ordering and positional-handling edge cases are subtle (this PR itself went through five follow-up commits during review to fix -e "" detection, - → stdin, the run-positional strip under node-argv0, and --parallel/--filter precedence). The implementation now looks correct and is well-tested, but it is not a mechanical change.
Other factors
- All prior inline feedback (mine and CodeRabbit's) is resolved; the two remaining items (no extension resolution for
bun --check foo→foo.js;bun test --checkaccepted-but-ignored) were explicitly deferred by the author with reasonable justification, and a maintainer may want to confirm those deferrals. - The bundled
wtf-bindings.cppchange is trivially correct but is an unrelated build-fix riding along. - Good test coverage (12 cases including no-execute guarantee, stdin,
-, TS/ESM/TLA, missing file,--evalconflict with exit 9,bun run --check). - No CODEOWNERS match for the touched paths.
StatusReady for maintainer review. Feature complete, rebased onto current main (head 205f030), 19 tests, all review threads resolved, docs entry added. CIHead 205f030 = fresh rebase onto main (2670e50) + node-argv0 test coverage (conflict: main added Prior builds on earlier heads only ever failed on lanes tagged Deferred nits (for maintainer to confirm)
|
2ab1374 to
bfbba29
Compare
afe44fc to
5bdf688
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
5bdf688 to
6dcc809
Compare
bun --check file.js parses the entry point with Bun's JS/TS parser and exits 0 (no output) on success or 1 with the syntax error, without booting the JS VM. Matches node --check semantics: reads stdin when no file is given, rejects combining with -e/-p, and honors --if-present. Before this, --check was an unknown flag and was silently skipped, so the file ran its top level. -c remains Bun's --config alias.
6dcc809 to
2670e50
Compare
Exercises the exec_as_if_node dispatch and the node-mode guard on the 'run' positional strip (a file literally named run must be checked, not stripped).
There was a problem hiding this comment.
Thanks for adding the argv0: "node" tests — that closes out the last coverage gap I flagged. This run found no new issues.
Because this adds a new user-facing CLI flag and there are a couple of explicitly-deferred items awaiting a maintainer call (the node -c remap under RunAsNodeCommand, and whether bun test --check should be rejected), a human sign-off would still be worthwhile.
What was reviewed: the three dispatch sites (exec_auto_or_run for Auto/Run, exec_as_if_node) all route to exec_check before VM boot; file-read uses cursor read_to_end_into so pipes/FIFOs work and the fd is closed via File's Drop; loader selection consults ctx.args.loaders before DEFAULT_LOADERS; --if-present short-circuits only on ENOENT; the --eval/--print conflict exits 9 to match Node. Tests now cover all three entry points including the PRETEND_TO_BE_NODE gate on the run-strip.
Extended reasoning...
Overview
This PR adds bun --check <file> (Node's node --check equivalent): parse the entry point for syntax errors and exit without executing. It touches:
src/options_types/context.rs— newsyntax_check: boolonRuntimeOptionssrc/runtime/cli/Arguments.rs—--checkparam entry, conflict check with--eval/--print(exit 9)src/runtime/cli/mod.rs— early dispatch toexec_checkfromexec_auto_or_runsrc/runtime/cli/run_command.rs— the ~100-lineexec_checkimplementation, plus a 3-line early-return inexec_as_if_nodetest/cli/run/check.test.ts— 17 tests (306 lines),docs/snippets/cli/run.mdx,completions/bun.zsh
Security risks
None identified. The command reads a file (or stdin) and runs it through the JS parser without booting the VM, resolving imports, or executing any code. The path is opened via sys::File::openat with O_RDONLY and no path traversal concerns beyond what bun <file> already has. Untrusted input reaches only the parser, which is already the attack surface for bun <file>.
Level of scrutiny
Medium-high. This is new user-facing CLI API surface, and the repo's REVIEW.md calls out "API design (adding or changing user-facing API surface)" as a situational section requiring extra care. The implementation itself is self-contained and well-tested, but two design questions were explicitly deferred by the author "for a maintainer call": (1) whether -c should remap to --check under RunAsNodeCommand (it currently stays --config, so node -c foo.js via the bun shim doesn't syntax-check), and (2) whether bun test --check should be explicitly rejected rather than silently accepted-and-ignored. Neither blocks the feature, but both are the kind of API-surface call a maintainer should make rather than a bot.
Other factors
This PR has been through many review iterations — every prior nit I raised (stdin -, --if-present, --loader override, ESPIPE on pipes, error-message wording, describe.concurrent, docs, and most recently argv0-node test coverage) has been addressed with a fix commit and a test. The current diff reflects all of those. The one still-unresolved inline comment (mine, on line 265, asking for argv0:"node" coverage) is now addressed by the two new tests at the end of check.test.ts (commit 205f030), so nothing is actually outstanding on my side. The remaining deferred items are the two maintainer-call questions above, which the author has explicitly flagged rather than silently dropped.
2 participants
What
Adds
bun --check <file>, equivalent to Node'snode --check/node -c: parse the entry point for syntax errors and exit without ever executing it.Behavior
bun --check <file>— parses<file>; exits 0 silently on success, prints the syntax error and exits 1 on failure.bun --checkwith no file — reads from stdin (likenode --check).bun run --check <file>— same.awaitand ESMimport/exportare accepted.--checkcombined with--eval/--printis rejected witheither --check or --eval can be used, not bothand exit code 9, matching Node.The short
-cspelling is not provided because-cis already--configin Bun.How
syntax_check: boolonRuntimeOptions, populated from a--checkentry inRUNTIME_PARAMS_.RunCommand::exec_check()reads the file (or stdin via a streamingread()loop —preadfails on pipes), picks a loader from the extension, and runsbun_js_parser::Parserdirectly against an emptyDefine. If the parse errors or the log has errors, it prints them and exits 1; otherwise exits 0.exec_auto_or_run(forbun/bun run) andexec_as_if_node(forbun-as-node) before VM boot.Rebased onto main several times; conflicts were always adjacent-insertion in
run_command.rs(most recently main'sdisable_default_env_filesline inexec_as_if_node, kept alongside the--checkearly return).exec_checkispub(crate)to satisfy main's-D unreachable-pub. An earlierwtf-bindings.cppassert fix was dropped because main fixed it independently.Tests
test/cli/run/check.test.tscovers: valid/invalid files, no-execute guarantee, TS syntax, ESM syntax, top-level await, stdin, missing file,--evalconflict, andbun run --check.no test proof · iteration 16 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/cli/run/check.test.ts