Bun.openInEditor: run vim, nvim and emacs directly on Linux and Windows - #38032
Bun.openInEditor: run vim, nvim and emacs directly on Linux and Windows#38032robobun wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe editor launch path now uses ChangesEditor launch behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 4:30 AM PT - Aug 16th, 2026
✅ @robobun, your commit b95af81dae6ceddfb1ead934e4b2924ecba35a2a passed in 🧪 To try this PR locally: bunx bun-pr 38032That installs a local version of the PR into your bun-38032 --bun |
|
Status: reproduced on Linux with bun 1.4.0 and on main by putting an |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — gating the opener prefix to macOS is the right shape; Linux/Windows now fall through to the same <binary> <file> argv every other editor already uses.
What was reviewed:
- Confirmed the removed
OPENERconstants were module-private toopen.rs;publish_command.rsandopen_urluse the separatepub(crate) const OPENERincli/mod.rs, which is untouched. - macOS argv is byte-identical to before (the
--argspush was already macOS-gated inside the block). - New tests cover all three detection paths (absolute, name-on-PATH,
$EDITOR), assert the exact argv via a recording stub, and would fail on the oldxdg-open <editor> <editor> <file>shape since the stub also stands in forxdg-open.
Extended reasoning...
Overview
This PR fixes Bun.openInEditor for vim/nvim/emacs on Linux and Windows. The bug: Editor::open unconditionally prefixed the argv with a platform "opener" (xdg-open on Linux, start on Windows) for these three editors, producing xdg-open <binary> <binary> <file> — a shape only meaningful on macOS where /usr/bin/open <binary> --args ... launches a terminal editor in Terminal.app. xdg-open takes exactly one argument and start is a cmd builtin, so nothing ever opened. The fix gates the entire opener block behind #[cfg(target_os = "macos")] and deletes the now-unused Windows/Linux OPENER constants (net -4 lines in src). On non-macOS these editors now fall through to push_arg!(binary) and the default match arm, giving <binary> <file> — the same argv already used for Editor::Other and every non-special-cased editor.
Security risks
None. This is developer-tooling code that spawns a user-configured editor binary with a user-provided file path via inherited stdio. No new inputs are introduced; the change only removes an argv prefix. The detached-thread spawn path is unchanged.
Level of scrutiny
Low-to-medium. The src change is a 4-line removal plus a #[cfg] attribute — mechanically simple with a clear rationale. I verified the deleted OPENER constants were private to open.rs (no pub); the open::OPENER referenced by publish_command.rs:1117 and open_url in cli/mod.rs:246 is a separate pub(crate) constant defined in mod.rs (the open.rs file is mounted as mod open_full and only re-exports Editor/EditorContext into pub mod open). macOS behavior is byte-for-byte unchanged: the --args push was already inside a #[cfg(target_os = "macos")] inner block, so lifting the cfg to the whole if produces identical argv on macOS.
Other factors
The tests are well-constructed per REVIEW.md: they use test.concurrent.each over the variant matrix (three editors × three detection paths sampled), install a recording shell stub for both the editor and xdg-open (on PATH and in cwd, so regression is caught however argv[0] resolves), poll for the atomic-mv'd output file rather than sleeping, drain stdout/stderr/exited concurrently, and assert the exact argv ([name, file, ""]). The stub writes ${0##*/} first, so the old broken argv would surface as xdg-open in position 0 and fail the assertion — the PR description confirms all five fail under USE_SYSTEM_BUN=1. The $EDITOR case correctly relies on detect_editor checking the explicit editor option before env vars, so any inherited EDITOR/VISUAL in bunEnv cannot interfere with the absolute-path and name cases. The PR description also flags the interaction with #31297 (an in-flight change to argv spawning) — that's a heads-up for whoever lands second, not a blocker here.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the poll-deadline nit from my earlier pass is addressed in b0c373b, and the comment-cop flags were resolved by shortening the opener comment to one line.
What was reviewed:
- Confirmed the removed
OPENERconstants inopen.rswere file-private and unused outside the now-macOS-gated block;publish_command.rsuses the separatemod.rsopen::OPENER, so Linux/Windows still compile. - Traced the argv on macOS before and after — byte-identical; on Linux/Windows vim/nvim/emacs now fall through to the
_ =>arm like every other editor. - Checked the test's env handling:
mergeWindowEnvsmakes the PATH/EDITOR overrides win overbunEnv, and the explicit{ editor }option takes precedence over any inherited$EDITOR/$VISUALin the non-$EDITOR rows.
Extended reasoning...
Overview
The PR gates the vim/nvim/emacs opener-wrapping block in Editor::open (src/runtime/cli/open.rs) behind #[cfg(target_os = "macos")] and deletes the now-unused Windows (start) and non-macOS (xdg-open) OPENER constants from that file. On Linux/Windows these three editors now build the same [<binary>, <file>] argv every other editor already uses via the _ => match arm. macOS keeps [/usr/bin/open, <binary>, --args, <binary>, <file>] unchanged. A new describe.skipIf(isMacOS) block in test/js/bun/util/open-in-editor-gc.test.ts installs argv-recording stubs for vim/nvim/emacs (and xdg-open on Linux) and asserts the exact argv shape across $EDITOR, { editor: "name" }, and { editor: "/abs/path" } selection.
Security risks
None. The change narrows what gets spawned (drops the opener wrapper on two platforms) rather than introducing any new external input into an argv. No user-controlled data reaches a shell — sync::spawn execs the argv directly.
Level of scrutiny
Low-to-medium. The source change is a 6-line net deletion that cfg-gates existing code; I verified the removed constants were file-private (the open::OPENER used by publish_command.rs lives in mod.rs's inline pub mod open, not in open.rs). The behavioral surface is Bun.openInEditor, a developer convenience API with no correctness dependents. The bulk of the diff is the test.
Other factors
- All three prior review threads are resolved: my unbounded-poll nit was fixed in b0c373b (run.js now throws "nothing was spawned" after 3s, so a regression fails the
stderrassertion instead of timing out), and the two comment-cop hits were addressed by trimming the block comment to one line in 18119d3. - The test is hermetic (own tempDir per row, stub prepended to PATH, no network), uses
mergeWindowEnvsto avoid the Windows PATH-casing race, and writes the argv record via tmp-file + rename on both platforms so the poll never observes a partial write. - The PR description documents that the fix was verified to fail with
USE_SYSTEM_BUN=1on Linux and against the release build on Windows, satisfying the fails-for-the-right-reason requirement.
There was a problem hiding this comment.
LGTM — the earlier poll-deadline nit is addressed in b0c373b, and cf50348 dedupes against the shared cli::open::OPENER in mod.rs.
What was reviewed
Editor::open: the#[cfg(target_os = "macos")]gate leaves the macOS argv byte-identical; Linux/Windows now fall through to the samepush_arg!(binary); … filepath already used forEditor::Other.- Confirmed
super::open::OPENERresolves to the inlinemod openincli/mod.rs(this file is loaded asopen_full), so removing the file-local duplicate constant leavesopen_urlandpublish_commandunaffected. - Test: stub writes atomically (tmp+mv) so the existence poll can't read a partial file;
mergeWindowEnvsavoids thePath/PATHcasing race; the 3s deadline surfaces spawn failures via thestderrassertion.
Extended reasoning...
Overview
Two files. In src/runtime/cli/open.rs, the vim/nvim/emacs branch of Editor::open that prefixed the argv with a platform "opener" (/usr/bin/open / xdg-open / start) is now gated behind #[cfg(target_os = "macos")]. The file-local OPENER constant is deleted and the macOS block references the pre-existing shared cli::open::OPENER in mod.rs instead (this file is mounted as mod open_full via #[path], so super::open is the inline module in mod.rs, not a self-reference). The rest of Editor::open is untouched, so on Linux/Windows the argv for these three editors is now <binary> <file> — the same shape the _ => arm already produces for unrecognised editors like nano. The macOS argv is byte-for-byte unchanged.
The test file gains a describe.skipIf(isMacOS) block with a test.each matrix over vim/nvim/emacs × ($EDITOR / name / absolute path), each installing a stub that records its own name plus argv into the target file, then asserting the record is exactly [editor, file]. On Linux the same stub also stands in for xdg-open, so the pre-fix argv (xdg-open <editor> <editor> <file>) would be captured and produce a visible diff rather than a silent pass.
Security risks
None. Bun.openInEditor is a developer convenience that already spawns whatever binary the user names in $EDITOR or the editor option; this PR only changes which arguments precede that binary in argv on non-macOS. No new user input is interpreted, no new privilege boundary is crossed, and no shell interpretation is introduced (spawn is via sync::spawn with an argv array, not a shell string).
Level of scrutiny
Low-to-medium. The Rust change is a net -12/+3 that gates an already-conditional block more tightly and removes a duplicate constant. The blast radius is one rarely-called API on two platforms where it was demonstrably broken (xdg-open rejects extra args; start is not spawnable), so the downside of the fix being wrong is bounded by "still doesn't open the editor". The design question — spawn directly vs. try to find a terminal emulator — is argued convincingly in the PR description (matches existing Editor::Other behaviour, avoids inventing Linux terminal-detection heuristics), and the current behaviour is strictly worse than either alternative.
Other factors
All three prior review threads are resolved: my unbounded-poll nit was fixed in b0c373b (3s deadline throwing an actionable message that surfaces via the expect(stderr).toBe("") assertion), and both comment-cop notes on the long code comment were addressed in 18119d3/cf50348 by moving the rationale to the PR description and the test's describe-block comment. The test follows harness conventions (tempDir with using, bunEnv/bunExe, mergeWindowEnvs for the Windows PATH-casing hazard, pipes drained concurrently, exit code asserted last), writes the record file atomically via tmp+mv so the poll never observes a partial write, and the author reports it fails under USE_SYSTEM_BUN=1 on Linux and against the unfixed release build on Windows. The rows are intentionally sequential with a stated reason (concurrent debug-build starts blew the default timeout), which satisfies the repo's "staying serial needs a stated reason" rule.
Editor::open built the argv for these editors as `<opener> <editor> <editor> <file>` on every platform. That shape is macOS's `open <editor> --args ...`, which runs the editor in a new Terminal window. xdg-open takes a single argument and exits 1 when given three, and `start` is a cmd.exe builtin that cannot be spawned at all, so on Linux and Windows these editors never opened. Only use the opener on macOS. Everywhere else the editor is spawned as `<editor> <file>`, the same way every other editor already is.
cf50348 to
b95af81
Compare
There was a problem hiding this comment.
LGTM — the earlier poll-deadline nit is addressed and the comment-cop feedback is resolved.
What was reviewed
super::open::OPENERresolves to the inlinepub mod openinsrc/runtime/cli/mod.rs(this file is mounted asopen_full), so the macOS path is byte-for-byte unchanged and the deleted localOPENERhad no other consumers.- On Linux/Windows the vim/nvim/emacs arms now fall through to the
_branch (<binary> <file>), matching whatEditor::Otheralready produces for unrecognized terminal editors. - Test: bounded 3s poll,
mergeWindowEnvsfor the Windows PATH-casing hazard, stub also installed asxdg-openso the unfixed argv shape shows up in the diff, sequential rows with a stated reason.
Extended reasoning...
Overview
Two files. src/runtime/cli/open.rs gates the vim/nvim/emacs "launch through the platform opener" block to #[cfg(target_os = "macos")] and deletes the file-local OPENER constant in favour of the shared one in cli::mod.rs's inline open module. On macOS the argv is identical to main (/usr/bin/open <binary> --args <binary> <file>); on Linux and Windows these three editors now fall through to the default _ match arm, producing <binary> <file> — exactly what every other editor variant (including Editor::Other) already gets. test/js/bun/util/open-in-editor-gc.test.ts gains a describe.skipIf(isMacOS) block with a test.each matrix over {vim, nvim, emacs} × {$EDITOR, name, absolute path} that installs a shell/.cmd stub which records its argv into the file it is asked to open.
Deleted-constant reachability
The removed OPENER in open.rs was file-local (private). Grep confirms the two remaining references (publish_command.rs:1100 and mod.rs:246) both point at cli::open::OPENER — the inline pub mod open in mod.rs, not this file. open.rs itself is mounted as mod open_full (mod.rs:224-225), so from inside it super::open::OPENER correctly reaches the shared constant. No broken references, no cfg-gated dead code left behind.
Security risks
None material. Bun.openInEditor already spawned a user-configured binary with a user-supplied file path; this PR only changes which binary is argv[0] on Linux/Windows (the editor itself instead of xdg-open/start). No new shell interpretation is introduced — the PR explicitly avoids cmd.exe /c start on Windows to sidestep re-tokenizing user paths, which is the safer choice. Arguments are passed via argv arrays, not shell strings.
Level of scrutiny
Low-to-moderate. Bun.openInEditor is a developer-convenience API, not a hot path or security boundary, and the current Linux/Windows behaviour is simply broken (nothing opens). The src change is ~10 lines, mostly deletion, and one platform is provably unchanged. The design question — spawning a TUI editor with inherited stdio from a detached thread while the main process continues — is what Editor::Other (e.g. nano) already does today, so this is consistency rather than new behaviour, and the PR description reasons through why a new-terminal-window approach was rejected on each platform.
Other factors
My prior inline nit (unbounded poll in run.js) was addressed in b0c373b with a 3s deadline and a diagnostic throw, verified by the author against the unfixed Windows build. The two comment-cop hits on the paragraph-long src comment were addressed by removing the comment entirely (b95af81 uses the shared constant with no inline explanation). All three inline threads are resolved. The test follows the file's established conventions (tempDir, bunEnv, pipes drained via Promise.all, stderr asserted before exitCode, sequential rows with a stated reason for not being concurrent), and the author reports it fails on both platforms without the src change.
1 participant
Problem
Bun.openInEditor(file)never opens anything on Linux or Windows when the editor resolves to vim, nvim or emacs ($EDITOR/$VISUAL,{ editor: "vim" },{ editor: "/usr/bin/vim" }).Editor::open(src/runtime/cli/open.rs:180on main) builds the argv for those three variants as<opener> <editor> <editor> <file>on every platform. That is the macOS form,/usr/bin/open <editor> --args ..., and it has been built this way for every platform since the editor support was first written (the Zigopen.zighad the same layout), so this is not a port regression.xdg-open, which takes one argument: when it runs, it printsxdg-open: unexpected argument '/usr/bin/vim'and exits 1. On main it does not even run, becauseauto_closeexecs the bare name without a PATH search (the spawn layer openInEditor: spawn the editor without spawnSync's signal forwarding #31297 fixes), so the spawn fails with ENOENT, or runs whateverxdg-openis in cwd. Whichever layer fails first, the editor never starts, and fixing the spawn alone only gets as far as the xdg-open error.start, a cmd.exe builtin rather than an executable, so the spawn fails and nothing opens (the same root cause as Open the browser via cmd.exe on Windows #32514, which changed the URL opener and left this call site for a follow-up).Fix
#[cfg(target_os = "macos")]and uses the existingcli::open::OPENER(/usr/bin/open) that the URL opener already uses; the per-platform copy inopen.rsis gone. On Linux and Windows vim, nvim and emacs are spawned as<editor> <file>.Editor::openalready produces for every editor outside this special case, including terminal editors it has no name for ({ editor: "/usr/bin/nano" }), and on Windows it is already what{ editor: "C:\\...\\vim.exe" }got, because an absolute path is classified by its exact basename andvim.exefalls intoOther. It also runs the editor the user configured;xdg-open <file>would not (it picks the desktop's handler for the file type).$TERMINAL,x-terminal-emulator, per-emulator-econventions). Windows does have one (cmd.exe /c start "" <editor> <file>), but it would send the user-supplied editor and file paths through cmd.exe's re-tokenizer and need the escaping or rejection logic that made Open the browser via cmd.exe on Windows #32514 leave this site out; a direct spawn needs none of that. This settles the call site Open the browser via cmd.exe on Windows #32514 deferred.openhands a Unix executable to Terminal.app, and--argsgoes to the launched application, not to the editor, which matches the note insrc/runtime/bake/DevServer.rs:1826("opens apple terminal + vim"). If someone with a Mac confirms the file does not open, the follow-up is to delete the block on macOS as well and let this test run there; I did not want to change macOS behaviour blind in this PR.test/js/bun/util/open-in-editor-gc.test.ts, "runs terminal editors directly". Each row installs a stub editor (shell script, or.cmdon Windows) that records its own name and arguments into the file it is asked to open, and asserts the record is exactly<editor>,<file>. Rows: vim, nvim and emacs via$EDITOR, vim via{ editor: "vim" }, and on Linux nvim via an absolute path (a.cmdstub's basename cannot select these editors). On Linux the stub is also installed asxdg-open, in cwd and on PATH so it is hit however argv[0] is resolved, so without the fix the rows fail showingxdg-open <editor> <editor> <file>being spawned. On Windows nothing is spawned without the fix, sorun.jsgives up after 3s with a "nothing was spawned" error that fails the stderr assertion.bun bd test test/js/bun/util/open-in-editor-gc.test.tspasses on Linux (7) and on Windows x64 (4; the two Linux-only tests skip). Without the src change the 5 Linux rows fail (USE_SYSTEM_BUN=1gives the same), and on Windows the 4 rows fail against the release build with the "nothing was spawned" message.Background
Bun.openInEditorfirst resolves an editor to an enum variant plus an absolute binary path (EditorContext::detect_editor: explicit absolute path, known name looked up on PATH,$EDITOR/$VISUAL, then a built-in preference list).Editor::openthen builds the argv and hands it to a detached thread that spawns it with inherited stdio and waits for it to exit./usr/bin/open(macOS) opens a file or application; for a Unix executable the handler is Terminal.app.xdg-open(freedesktop) opens one file or URL with its default handler and has no way to pass arguments to a program.startis a cmd.exe builtin of the formstart ["title"] program [args]; it can open a console program in a new window, but only when invoked throughcmd.exe /c.mergeWindowEnvsbecausebunEnvspells the variablePathon Windows, and which of two differently cased PATH keys a child sees is currently not deterministic (Bun.spawn: de-duplicate case-insensitive env names on Windows #37382). The rows are not concurrent because five debug builds starting at once ran past the default per-test timeout on a loaded machine; sequentially each row takes about half a second.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/util/open-in-editor-gc.test.ts