Bun.openInEditor: throw when no editor is found instead of spawning ""

[dylan-conway]

What does this PR do?

Bun.openInEditor(file, { editor: "does-not-exist" }) — or a call with no editor when nothing can be auto-detected — was meant to throw Could not find editor "…" / Failed to auto-detect editor, but never did. EditorContext::detect_editor stores Some(Editor::None) once it has looked everywhere (name, EDITOR/VISUAL, PATH, fallback app paths) and found nothing, so it doesn't search again; the two is_none() checks in open_in_editor therefore never fired and the call fell through to Editor::open with an empty binary path, which starts a detached thread that spawn_syncs argv[0] = "" and swallows the failure. From JS it looked like success. This treats Editor::None as "not found" at those checks (EditorContext::found()), making both errors reachable; nothing is spawned.

Why now: test/js/bun/util/open-in-editor-gc.test.ts › "survives re-entrant calls from option getters" (added in #36920) has been timing out in the Linux parallel lane since the day it landed (debian/ubuntu/alpine; builds 89333, 90274, 90329, 90444, 90571). It was written on the assumption that with an empty PATH and no EDITOR those calls are inert; in fact each run started eight of those editor threads racing the fixture's exit. I could not reproduce the hang itself locally (0/250, idle and loaded), so I can't claim more than: the fixture now does no spawning at all, which is what the test intended, and the API reports the failure it always meant to.

The test keeps its re-entrancy coverage and now asserts the four error messages instead of swallowing them.

How did you verify your code works?

Linux x64: with the current canary the updated test fails (Received: "opened" ×4 — the silent-success bug); with this branch bun bd test test/js/bun/util/open-in-editor-gc.test.ts → 2 pass. (The test is Linux-only because macOS auto-detect probes /Applications.)

---

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

[dylan-conway]

@robobun adopt

[robobun]

Adopted. Verified the bug with the current canary (test prints "opened" x4, the silent spawn of an empty argv[0]). Building and running the branch test now, will take it from here.

[coderabbitai]

Walkthrough

EditorContext::found() now filters unsuccessful editor detection. Bun.openInEditor uses this result for explicit and automatic editor selection. The test verifies editor lookup failures for all four option variants.

Changes

Editor detection flow

Layer / File(s)	Summary
Filter unsuccessful editor detection src/runtime/cli/open.rs, src/runtime/api/BunObject.rs	EditorContext::found() excludes Editor::None. Bun.openInEditor uses the filtered result during explicit and automatic editor selection.
Validate missing-editor failures test/js/bun/util/open-in-editor-gc.test.ts	The test logs caught error messages and expects editor lookup failures for explicit and automatic detection paths.

Suggested reviewers: robobun, jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly summarizes the main change: Bun.openInEditor now throws when no editor is found.
Description check	✅ Passed	The description completes both required sections and explains the bug, implementation, test changes, and Linux verification results.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — straightforward fix making the two "editor not found" error paths in Bun.openInEditor actually reachable.

What was reviewed:

• Traced detect_editor → confirmed it stores Some(Editor::None) as a search-done sentinel, so the old is_none() checks at both throw sites were dead; found() correctly filters the sentinel.
• Walked all four test variants through the re-entrancy path (inner reenter() → restore → outer call) and each produces the asserted message; the re-entrancy invariant from #36920 is still exercised (a panic on borrow_mut() would fail the test).
• Checked other EditorContext/.editor consumers (repl.rs, repl_command.rs, bunfig.rs) — none touched, found() is only called from open_in_editor.

Extended reasoning...

Overview

Three-file change: a 3-line EditorContext::found() helper in src/runtime/cli/open.rs that filters the Editor::None sentinel out of Option<Editor>; three call-site swaps in open_in_editor (src/runtime/api/BunObject.rs) from edit.editor to edit.found() plus .or → .or_else; and a test update that replaces four swallowed-catch "ok" assertions with the exact error messages.

Security risks

None. This narrows behavior — a call that previously silently spawned a detached thread with argv[0] = "" now throws before spawning. No new user input reaches any syscall; the only new code path is an early return Err(throw(...)).

Level of scrutiny

Low-to-medium. Bun.openInEditor is a developer-convenience utility, not a hot path or a security boundary. The fix is a sentinel-interpretation correction at three call sites, backed by a helper on the type that owns the sentinel's meaning. The .or → .or_else change is behaviorally identical (found() is a pure read). I traced every branch of open_in_editor after the change: the named-editor path now throws-and-restores on failure (previously the restore was dead too), the cached-name path falls through to edit.found() correctly, and the auto-detect path throws on Some(Editor::None) whether freshly detected or cached from a prior call.

Other factors

• The updated test is strictly stronger: it keeps the re-entrancy coverage (getters/toString still call Bun.openInEditor recursively before the RefCell borrow) and now asserts exact error text for both throw sites instead of catch {}. Verified the four expected lines by hand-tracing state across the reenter() calls and the *edit = prev restore.
• found() is pub(crate) and only called from open_in_editor; grepped for other EditorContext consumers and none read .editor in a way this affects.
• PR description is candid about what was and was not reproduced (the CI hang), and the claimed verification (updated test fails on canary with "opened" ×4, passes on the branch) matches what the diff would produce.

[claude]

LGTM — small, well-scoped fix that makes the two "editor not found" error paths in Bun.openInEditor actually reachable.

What was reviewed:

• Traced detect_editor → confirmed it terminates with Some(Editor::None) as a "searched, found nothing" sentinel, so the old is_none() checks in open_in_editor were unreachable and control fell through to Editor::open("", …).
• Checked all EditorContext consumers (only BunObject.rs and open.rs itself) — no other site reads .editor directly and needs the same treatment; auto_detect_editor correctly still gates on raw .is_none() to preserve caching.
• Walked the four test variants against the state-restore logic (*edit = prev on the explicit-name failure path, no restore on the auto-detect path) — the asserted message sequence matches, and re-entrancy coverage from #36920 is preserved since the getters still call Bun.openInEditor before the RefCell borrow.

Extended reasoning...

Overview

Adds EditorContext::found() (filters the Some(Editor::None) sentinel to None) and swaps three edit.editor reads in open_in_editor for it. detect_editor writes Some(Editor::None) when the search comes up empty so it is not repeated; the old is_none() checks therefore never fired and the code fell through to spawning a detached thread with argv[0] = "". The updated test asserts the four now-reachable error messages instead of swallowing them, and the PR description ties this to a flaky Linux CI test that was unintentionally spawning eight of those threads per run.

Security risks

None. Bun.openInEditor is a developer utility; the change only adds early-exit error paths and removes an unintended spawn_sync(""). No new input parsing, no memory or lifetime changes.

Level of scrutiny

Low. Three-line helper plus three call-site swaps in a non-critical path. The .or(edit.editor) → .or_else(|| edit.found()) change is semantically the intended fix (and lazily evaluated, which is fine). I confirmed via grep that EditorContext has no other consumers in the repo, so there is no sibling site left un-fixed. auto_detect_editor intentionally keeps its raw self.editor.is_none() guard — that is the cache check, and switching it to found() would defeat the "do not search again" behavior the sentinel exists for.

Other factors

The test still exercises the re-entrancy fix from #36920 (getters call Bun.openInEditor during option coercion, before the EDITOR_CONTEXT RefCell borrow), and now additionally asserts exact error messages — a strictly stronger invariant than the previous "ok" ×4. I traced the *edit = prev restore on the explicit-editor failure path and the lack of restore on the auto-detect path against the four variants; the expected output sequence (Could not find editor … ×2, Failed to auto-detect editor ×2) is correct given empty PATH and unset EDITOR/VISUAL. The test remains Linux-gated for the same reason as before (macOS probes /Applications).