fix(moshscript): don't claim mosh() launched a browser it never opened - #223
Merged
ralyodio merged 1 commit intoAug 3, 2026
Merged
Conversation
openBrowser() returns true whenever spawn() starts, but a missing opener is
reported asynchronously as an 'error' event on the child, so the surrounding
try/catch only ever catches a synchronous spawn failure. mosh() gated
"launched in your browser" on that return value and printed it on any box
with a display, including one with no xdg-open installed at all.
Word the line as the attempt it is, matching how dns.mjs ("opening <url>")
and auth.mjs ("opening your browser…") already phrase their own opens, and
document the return value's real contract on openBrowser itself. The playlist
URL is already printed above the line, so there is still something to fall
back on when nothing appears.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
On a desktop with no browser opener installed,
mosh()tells you it launched a browser. Nothing launched.openBrowser()insrc/commands.mjsreturnstrueas soon asspawn()returns, andmosh()gates its success line on that:A missing opener is not a synchronous throw. Node reports it as an async
'error'event on the child, which arrives afteropenBrowserhas already returned. The surroundingtry/catchonly ever catches a synchronous spawn failure, so for the common ENOENT case it is dead code and the function returnstrueregardless.Reproduced before touching anything
Same script, same box, with and without an opener on
PATH(DISPLAYset sohasDesktop()is true):Byte-identical to the run where
xdg-openreally is installed. And the ordering, directly:The fix
Word the line as the attempt it actually is, and document the return value's real contract on
openBrowseritself.This follows the two call sites that already get it right, rather than inventing a phrasing:
src/dns.mjs:1182—out("opening ${pitUrl}")src/auth.mjs:101—"opening your browser to authorize the moshcode CLI…", plus"if it doesn't open, visit: <url>"src/open-url.mjsalso already documents the shared copy as "returns whether it was attempted".commands.mjswas the only caller upgrading "attempted" to "launched". The playlist URL is printed on the line above, so there is still something to fall back on when nothing appears.What I deliberately did not do
'error'handler does fire before the process exits, so printing "actually, that failed" is possible. I left it out on purpose: it would put unordered output after the summary line on a fire-and-forget path, and neitherdns.mjsnorauth.mjsdoes it. Reporting the attempt honestly is enough here.open-url.mjs. That file's own comment scopes it out ("Folding those two in is a separate change"), so this PR leaves it alone.hasDesktop()incommands.mjsstill lacks the SSH check thatcanOpenBrowser()has, so a display-forwarded SSH session gets the same optimistic line. That is the same deferred fold-in, so it is out of scope here — flagging it rather than silently widening the diff.Tests
New
test/commands-mosh-open.test.mjs, 5 tests. They drive the real (non-dry) path with an emptyPATH, so the opener genuinely cannot resolve on any platform.With the fix stashed, the intended two fail and all three controls pass:
Controls 3-5 pin that the fix does not silence the line, break
--dry-run, or start mentioning a browser on a headless box. Test 5 skips on darwin/win32, wherehasDesktop()is unconditionally true.Full suite on this branch: 910 tests, 721 pass, 0 fail (905/716 on
mainat 053ec57 / v0.16.6).