-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix: preserve -- in process.argv when running script files
#30945
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { spawnSync } from "bun"; | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { rmSync, writeFileSync } from "fs"; | ||
| import { bunEnv, bunExe, bunRun, isWindows } from "harness"; | ||
| import { bunEnv, bunExe, bunRun, isWindows, tempDir } from "harness"; | ||
|
|
||
| let cwd: string; | ||
|
|
||
|
|
@@ -30,3 +30,110 @@ test.if(isWindows)("[windows] A file in drive root runs", () => { | |
| rmSync(path); | ||
| } | ||
| }); | ||
|
|
||
| // https://github.com/oven-sh/bun/issues/13984 | ||
| // Node.js preserves '--' in process.argv when running a script file. | ||
| // Bun was stripping it in the CLI arg parser's stop_after_positional_at codepath. | ||
| describe("should preserve '--' in process.argv", () => { | ||
| test("bun -e <code> -- rest (separator consumed)", () => { | ||
| 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); | ||
|
Comment on lines
+39
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial | ⚡ Quick win Add Bun’s subprocess failure-diagnostics guard before exit-code assertions. Please add Based on learnings: when spawning subprocesses in Bun tests, place the stderr guard immediately before the final Also applies to: 57-69, 77-87 🤖 Prompt for AI Agents |
||
| }); | ||
|
|
||
| test("bun run script.js -- rest (preserved in argv)", () => { | ||
| using dir = tempDir("test-double-dash", { | ||
| "test.js": "console.log(JSON.stringify(process.argv))", | ||
| }); | ||
|
|
||
| const { exitCode, stdout, stderr } = spawnSync({ | ||
| cmd: [bunExe(), "run", "test.js", "--", "rest", "--foo=bar"], | ||
| cwd: String(dir), | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const argv = JSON.parse(stdout.toString()); | ||
| // argv[0] is the bun executable, argv[1] is the script path | ||
| // '--' must be preserved in argv[2], matching Node.js behavior | ||
| expect(argv.slice(2)).toEqual(["--", "rest", "--foo=bar"]); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("bun run script.js -- with multiple double dashes", () => { | ||
| using dir = tempDir("test-double-dash-multi", { | ||
| "test.js": "console.log(JSON.stringify(process.argv))", | ||
| }); | ||
|
|
||
| const { exitCode, stdout, stderr } = spawnSync({ | ||
| cmd: [bunExe(), "run", "test.js", "--", "abc", "--", "def"], | ||
| cwd: String(dir), | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const argv = JSON.parse(stdout.toString()); | ||
| expect(argv.slice(2)).toEqual(["--", "abc", "--", "def"]); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("bun run <pkg-script> -- args (separator consumed, npm compat)", () => { | ||
| using dir = tempDir("test-double-dash-pkg", { | ||
| "echo.js": "console.log(JSON.stringify(process.argv))", | ||
| "package.json": JSON.stringify({ | ||
| scripts: { | ||
| echo: `${bunExe()} echo.js`, | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const { exitCode, stdout, stderr } = spawnSync({ | ||
| cmd: [bunExe(), "run", "echo", "--", "rest", "value"], | ||
| cwd: String(dir), | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| const lines = stdout.toString().trim().split("\n"); | ||
| const argv = JSON.parse(lines[lines.length - 1]); | ||
| // For package scripts, '--' is consumed as an npm-style separator | ||
| // (matching npm/yarn), so only the args after it are forwarded. | ||
| // The inner script sees: [bun, echo.js, rest, value] | ||
| expect(argv.slice(2)).toEqual(["rest", "value"]); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
|
|
||
| test("bun run <pkg-script> -- args strips separator for Bun shell $1", () => { | ||
| using dir = tempDir("test-double-dash-pkg-shell", { | ||
| "package.json": JSON.stringify({ | ||
| scripts: { | ||
| show: 'echo "$1|$2" #', | ||
| }, | ||
| }), | ||
| }); | ||
|
|
||
| const { exitCode, stdout, stderr } = spawnSync({ | ||
| cmd: [bunExe(), "run", "show", "--", "rest", "value"], | ||
| cwd: String(dir), | ||
| env: bunEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
|
|
||
| // Bun shell reads $1/$2 from ctx.passthrough; the leading '--' | ||
| // must be stripped so $1 = "rest", $2 = "value" (npm compat). | ||
| expect(stdout.toString().trim().split("\n").at(-1)).toBe("rest|value"); | ||
| expect(exitCode).toBe(0); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 This change is at the shared clap layer, so it also changes
bun run <package.json-script> -- argsandbun run <bin> -- args, not just script files. After this PR,bun run dev -- --port 3000(wheredevis e.g."vite") appends-- --port 3000to the shell command instead of--port 3000, which diverges from npm and breaks the very commonnpm run script -- argshabit. The leading--should be stripped in the package-script / binary / filter / multi-run consumers (or the preservation moved to wherevm.argvis populated) rather than removed wholesale from clap.Extended reasoning...
What changed and why it leaks
The fix removes the leading-
--strip from thestop_after_positional_atcodepath incomptime.rs. That codepath producespassthrough_positionals, which is exposed viaargs.remaining()and assigned once toctx.passthroughatsrc/runtime/cli/Arguments.rs:944.ctx.passthroughis then consumed by everybun runexecution 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/--sequentialNone of these downstream consumers strip a leading
--(grep forb"--"inrun_command.rsreturns no relevant hits), so the new--flows straight through.Step-by-step proof:
bun run dev -- --port 3000Given a
package.jsonwith"scripts": { "dev": "vite" }:RunCommandparses withstop_after_positional_at = 2(Arguments.rs:760). Positionals =["run", "dev"], then parsing stops.stream.iter.remain()=["--", "--port", "3000"]."--"was dropped →passthrough = ["--port", "3000"].passthrough = ["--", "--port", "3000"].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.:273, each part is appended space-separated to the script body:copy_script = "vite -- --port 3000"(was"vite --port 3000"before).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.jsnow execs["prettier", "--", "file.js"]instead of["prettier", "file.js"]. And inmulti_run.rs:778,bun run --parallel a b -- cwould 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 — matchednpm run <script> -- <args>semantics, where--is a CLI-level separator that npm consumes. There is no compensating strip anywhere downstream ofctx.passthrough; the only place--was ever removed is the line this PR deletes. The PR's new tests only exercise the script-file →process.argvpath; there is no test coveringbun run <pkg-script> -- argsorbun 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"--"frompassthroughat the four non-vm.argvconsumption points (run_package_script_foregroundcall site atrun_command.rs:2515,run_binary_without_bunx_pathat:2141,filter_run.rs:820,multi_run.rs:778). Alternatively, leave clap as-is onmainand instead preserve--only wherevm.argvis populated (run_command.rs:963/:1190). Either way, please add a test forbun run <pkg-script> -- argto lock the behavior.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in latest push. Now stripping -- at the downstream consumers instead of at the clap layer.