fix: preserve -- in process.argv when running script files#30945
fix: preserve -- in process.argv when running script files#30945remorses wants to merge 1 commit into
-- in process.argv when running script files#30945Conversation
WalkthroughParser now preserves a leading ChangesProcess argv
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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[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 `@test/cli/run/run_command.test.ts`:
- Around line 39-49: Add a stderr guard before the exit-code assertion for each
spawnSync case: when checking the child exit you must insert if (exitCode !== 0)
{ expect(stderr.toString()).toBe(""); } immediately before each
expect(exitCode).toBe(0) so failures print stderr; update the block that calls
spawnSync (referencing spawnSync, exitCode, stderr, stdout) and the other
similar test blocks noted (the cases around the test assertions) to include that
guard right before the final exit-code assertion.
🪄 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: 554e1cf9-df15-450c-a7c3-1d1a15da9202
📒 Files selected for processing (2)
src/clap/comptime.rstest/cli/run/run_command.test.ts
| const { exitCode, stdout, stderr } = spawnSync({ | ||
| cmd: [bunExe(), "-e", "console.log(JSON.stringify(process.argv))", "--", "rest", "--foo=bar"], | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const argv = JSON.parse(stdout.toString()); | ||
| // With -e, '--' is consumed as a separator (same as Node.js) | ||
| expect(argv.slice(1)).toEqual(["rest", "--foo=bar"]); | ||
| expect(exitCode).toBe(0); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add Bun’s subprocess failure-diagnostics guard before exit-code assertions.
Please add if (exitCode !== 0) { expect(stderr.toString()).toBe(""); } immediately before each expect(exitCode).toBe(0) so failures surface stderr clearly in CI diffs.
Based on learnings: when spawning subprocesses in Bun tests, place the stderr guard immediately before the final expect(exitCode).toBe(0) assertion for better failure output.
Also applies to: 57-69, 77-87
🤖 Prompt for 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.
In `@test/cli/run/run_command.test.ts` around lines 39 - 49, Add a stderr guard
before the exit-code assertion for each spawnSync case: when checking the child
exit you must insert if (exitCode !== 0) { expect(stderr.toString()).toBe(""); }
immediately before each expect(exitCode).toBe(0) so failures print stderr;
update the block that calls spawnSync (referencing spawnSync, exitCode, stderr,
stdout) and the other similar test blocks noted (the cases around the test
assertions) to include that guard right before the final exit-code assertion.
remorses
force-pushed
the
claude/fix-double-dash-argv
branch
from
4093453 to
29e71a2
Compare
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if param.names.long.is_none() && param.names.short.is_none() { | ||
| pos.push(arg.value.unwrap()); | ||
| if opt.stop_after_positional_at > 0 && pos.len() >= opt.stop_after_positional_at { | ||
| let mut remaining_ = stream.iter.remain(); | ||
| // PORT NOTE: Zig called `bun.span` (NUL-scan) on `[:0]const u8` argv | ||
| // entries. Our `ArgIter` already yields sized `&[u8]`, so `span` is a | ||
| // no-op and is dropped. | ||
| let first: &[u8] = if !remaining_.is_empty() { | ||
| remaining_[0] | ||
| } else { | ||
| b"" | ||
| }; | ||
| if !first.is_empty() && first == b"--" { | ||
| remaining_ = &remaining_[1..]; | ||
| } | ||
| // Preserve all remaining args including '--' so it | ||
| // appears in process.argv, matching Node.js behavior. | ||
| // See: https://github.com/oven-sh/bun/issues/13984 | ||
| let remaining_ = stream.iter.remain(); | ||
|
|
||
| passthrough_positionals.reserve_exact(remaining_.len()); | ||
| for arg_ in remaining_ { |
There was a problem hiding this comment.
🔴 This change is at the shared clap layer, so it also changes bun run <package.json-script> -- args and bun run <bin> -- args, not just script files. After this PR, bun run dev -- --port 3000 (where dev is e.g. "vite") appends -- --port 3000 to the shell command instead of --port 3000, which diverges from npm and breaks the very common npm run script -- args habit. The leading -- should be stripped in the package-script / binary / filter / multi-run consumers (or the preservation moved to where vm.argv is populated) rather than removed wholesale from clap.
Extended reasoning...
What changed and why it leaks
The fix removes the leading--- strip from the stop_after_positional_at codepath in comptime.rs. That codepath produces passthrough_positionals, which is exposed via args.remaining() and assigned once to ctx.passthrough at src/runtime/cli/Arguments.rs:944. ctx.passthrough is then consumed by every bun run execution mode, not just script-file execution:
run_command.rs:963/:1190—vm.argv = take(ctx.passthrough)→process.argvfor script files (the intended fix target)run_command.rs:2515→run_package_script_foreground(loop at:273) — appends each passthrough item to the shell command for package.json scriptsrun_command.rs:2141-2145—run_binary_without_bunx_pathbuildsargv = [executable, ...passthrough]for node_modules/.bin executablesfilter_run.rs:820-829— same shell-concat pattern for--filtermulti_run.rs:778-781— pushes each passthrough item as a script name for--parallel/--sequential
None of these downstream consumers strip a leading -- (grep for b"--" in run_command.rs returns no relevant hits), so the new -- flows straight through.
Step-by-step proof: bun run dev -- --port 3000
Given a package.json with "scripts": { "dev": "vite" }:
RunCommandparses withstop_after_positional_at = 2(Arguments.rs:760). Positionals =["run", "dev"], then parsing stops.stream.iter.remain()=["--", "--port", "3000"].- Before this PR: leading
"--"was dropped →passthrough = ["--port", "3000"]. - After this PR:
passthrough = ["--", "--port", "3000"].
- Before this PR: leading
Arguments.rs:944:ctx.passthrough = ["--", "--port", "3000"].run_command.rsresolvesdevas a package.json script (not a file), clonesctx.passthroughat:2515, and callsrun_package_script_foreground.- At
:273, each part is appended space-separated to the script body:copy_script = "vite -- --port 3000"(was"vite --port 3000"before). - The shell runs
vite -- --port 3000. Vite (and most CLIs) treats--as end-of-options, so--portand3000become positional file arguments instead of the port option — the user's dev server starts on the default port (or errors on the unknown entry).
The same trace applies to run_binary_without_bunx_path: bun run prettier -- file.js now execs ["prettier", "--", "file.js"] instead of ["prettier", "file.js"]. And in multi_run.rs:778, bun run --parallel a b -- c would now push "--" as a script name → Script not found: --.
Why nothing prevents it
The old behavior — stripping the first -- separator before forwarding to scripts/binaries — matched npm run <script> -- <args> semantics, where -- is a CLI-level separator that npm consumes. There is no compensating strip anywhere downstream of ctx.passthrough; the only place -- was ever removed is the line this PR deletes. The PR's new tests only exercise the script-file → process.argv path; there is no test covering bun run <pkg-script> -- args or bun run <bin> -- args, so CI won't catch this.
Impact
This is a user-visible regression in an extremely common workflow (bun run <script> -- <flags> is muscle memory for anyone coming from npm/yarn). It silently changes how flags are delivered to dev servers, formatters, test runners, etc., and diverges from npm. The PR title/description only mention script files, so this appears unintended.
Suggested fix
Keep the clap change (it's correct for process.argv), and strip a single leading "--" from passthrough at the four non-vm.argv consumption points (run_package_script_foreground call site at run_command.rs:2515, run_binary_without_bunx_path at :2141, filter_run.rs:820, multi_run.rs:778). Alternatively, leave clap as-is on main and instead preserve -- only where vm.argv is populated (run_command.rs:963/:1190). Either way, please add a test for bun run <pkg-script> -- arg to lock the behavior.
There was a problem hiding this comment.
Fixed in latest push. Now stripping -- at the downstream consumers instead of at the clap layer.
| test("bun script.js -- rest", () => { | ||
| const { exitCode, stdout, stderr } = spawnSync({ | ||
| cmd: [bunExe(), "-e", "console.log(JSON.stringify(process.argv))", "--", "rest", "--foo=bar"], |
There was a problem hiding this comment.
🟡 nit: this test is named bun script.js -- rest but actually invokes bun -e (and asserts that -- is stripped, not preserved). Consider renaming to something like bun -e <code> -- rest (separator consumed) so the test name matches what it exercises. The third test has a similar minor mismatch — it's named bun script.js -- ... but invokes bun run test.js.
Extended reasoning...
What's wrong
The test on line 38 is named "bun script.js -- rest", which implies it tests the AutoCommand / script-file codepath (bun script.js). But the actual command on line 40 is:
cmd: [bunExe(), "-e", "console.log(JSON.stringify(process.argv))", "--", "rest", "--foo=bar"]This exercises the -e/eval codepath instead. The test body even acknowledges this on line 47: "With -e, '--' is consumed as a separator (same as Node.js)" — and the assertion on line 48 checks that -- is absent from argv, which is the opposite of what the surrounding describe("should preserve '--' in process.argv") block title suggests.
Why it's confusing
Walking through what a reader sees:
describeblock says: "should preserve '--' in process.argv"- First test name says: "bun script.js -- rest"
- A reader skimming CI output for a failure on this test will assume it's the script-file passthrough case (the thing this PR actually fixes)
- But the test body runs
bun -e, hits the streaming-parser separator path (wherestop_after_positional_atnever fires), and asserts--is stripped — a totally different codepath with the opposite expected behavior
So if this test fails in CI, the failure message (bun > should preserve '--' in process.argv > bun script.js -- rest) actively misleads about which codepath broke.
The third test (line 72, "bun script.js -- with multiple double dashes") has a milder version of the same issue: it's named bun script.js but actually runs bun run test.js. That distinction matters less since both go through stop_after_positional_at, but it's still inaccurate.
Impact
None on correctness — the assertions are valid for what's actually being run, and the test passes. This is purely a readability/debuggability concern.
Suggested fix
Rename the first test to something like "bun -e <code> -- rest (separator consumed)" and the third to "bun run script.js -- with multiple double dashes". Alternatively, change the first test's body to actually run a script file via bun script.js (no run subcommand), which would also add coverage for the AutoCommand path that currently isn't tested here.
There was a problem hiding this comment.
Fixed: renamed tests to match what they actually exercise.
remorses
force-pushed
the
claude/fix-double-dash-argv
branch
from
29e71a2 to
67f5149
Compare
Node.js preserves `--` in process.argv when running a script file
(e.g. `node script.js -- rest` gives `["node", "script.js", "--", "rest"]`).
Bun was stripping it in the CLI arg parser.
The bug was in both `src/clap/comptime.rs` and `src/clap/comptime.zig` in the
`stop_after_positional_at` codepath. After identifying the first positional arg
(the script filename), the code collected remaining args as passthrough but
explicitly checked if the first remaining arg was `--` and skipped it. This
caused `--` to disappear from process.argv.
The fix has two parts:
1. Remove the `--` stripping from the clap layer (`comptime.rs`/`.zig`) so
`ctx.passthrough` preserves all remaining args including `--`. This means
`process.argv` (set via `vm.argv = ctx.passthrough`) now matches Node.js.
2. Strip leading `--` at the non-process.argv consumers that forward args to
package scripts or binaries, matching npm/yarn `--` separator semantics:
- `run_package_script_foreground` call site (package.json scripts)
- `run_package_script_foreground` Bun shell path (swap ctx.passthrough
for shell $1/$2 positional expansion)
- `run_binary_without_bunx_path` (node_modules/.bin binaries)
- `BunXFastPath::try_launch` command-line building (Windows bunx)
- `filter_run.rs` (--filter scripts)
- `multi_run.rs` (--parallel/--sequential script names)
The `-e`/`--eval` case is unaffected because `--` is consumed earlier by the
streaming parser as a positional separator (matching Node.js behavior where
`node -e "code" -- args` also strips `--`).
Fixes oven-sh#13984
Session: ses_1c8657c16ffev9oftdw66OjYxq
remorses
force-pushed
the
claude/fix-double-dash-argv
branch
from
67f5149 to
08fe43e
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/cli/run/run_command.test.ts (1)
38-50:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd stderr guard before each exitCode assertion.
All five test cases are missing the stderr diagnostic guard. Add
if (exitCode !== 0) { expect(stderr.toString()).toBe(""); }immediately before eachexpect(exitCode).toBe(0)assertion (lines 49, 69, 87, 114, 137) to surface stderr clearly in CI diffs when tests fail.Based on learnings: when spawning subprocesses in Bun tests, place the stderr guard immediately before the final
expect(exitCode).toBe(0)assertion for better failure output.Also applies to: 52-70, 72-88, 90-115, 117-138
🤖 Prompt for 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. In `@test/cli/run/run_command.test.ts` around lines 38 - 50, Add a stderr diagnostic guard immediately before each final exit assertion in the tests that call spawnSync: for each test that does const { exitCode, stdout, stderr } = spawnSync(...) (notably the "bun -e <code> -- rest (separator consumed)" test and the four other similar tests), insert if (exitCode !== 0) { expect(stderr.toString()).toBe(""); } directly before expect(exitCode).toBe(0) so failed subprocesses surface stderr in CI diffs; update all occurrences that reference exitCode/stderr from spawnSync accordingly.
🤖 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.
Duplicate comments:
In `@test/cli/run/run_command.test.ts`:
- Around line 38-50: Add a stderr diagnostic guard immediately before each final
exit assertion in the tests that call spawnSync: for each test that does const {
exitCode, stdout, stderr } = spawnSync(...) (notably the "bun -e <code> -- rest
(separator consumed)" test and the four other similar tests), insert if
(exitCode !== 0) { expect(stderr.toString()).toBe(""); } directly before
expect(exitCode).toBe(0) so failed subprocesses surface stderr in CI diffs;
update all occurrences that reference exitCode/stderr from spawnSync
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 63168d13-9626-4884-8194-90d7312a4c2b
📒 Files selected for processing (6)
src/clap/comptime.rssrc/clap/comptime.zigsrc/runtime/cli/filter_run.rssrc/runtime/cli/multi_run.rssrc/runtime/cli/run_command.rstest/cli/run/run_command.test.ts
2 participants
What does this PR do?
Fixes #13984
Node.js preserves
--inprocess.argvwhen running a script file:The fix has two parts:
1. Clap layer (
src/clap/comptime.rs+.zig): removed the--stripping from thestop_after_positional_atcodepath soctx.passthroughpreserves all remaining args. This meansprocess.argv(set viavm.argv = ctx.passthrough) now includes--, matching Node.js.2. Non-process.argv consumers: added
strip_leading_double_dash()at the sites that forward passthrough args to package scripts or binaries, preserving the npm/yarn--separator semantics:run_package_script_foregroundcall site (package.json scripts)run_binary_without_bunx_path(node_modules/.bin binaries)BunXFastPath::try_launchcommand-line building (Windows bunx)filter_run.rs(--filterscripts)multi_run.rs(--parallel/--sequentialscript names)The
-e/--evalcase is unaffected;--is consumed earlier by the streaming parser as a positional separator (matching Node.js).How did you verify your code works?
Added tests in
test/cli/run/run_command.test.tscovering:bun -e "code" -- args(separator consumed, matches Node.js)bun run script.js -- rest(preserved in argv)bun run script.js -- abc -- def(multiple--all preserved)bun run <pkg-script> -- args(separator consumed, npm compat)Verified tests fail against system Bun (reproduces the bug) before the fix.