Repository navigation
fix(cli): use rundll32 instead of cmd.exe for Windows browser fallback in dashboard command - #7844
Conversation
There was a problem hiding this comment.
Code Review
This pull request replaces the Windows browser fallback command in bin/cli/commands/dashboard.mjs with rundll32 instead of cmd.exe to avoid spawning an unnecessary shell, and adds a corresponding changelog entry and unit tests. The review feedback correctly identifies a testing anti-pattern where the unit tests duplicate the production logic instead of importing and testing the actual production code, which fails to verify the real implementation and maintain proper test coverage.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| import test from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
|
|
||
| /** | ||
| * Replicate the openFallback logic from bin/cli/commands/dashboard.mjs | ||
| * to verify the correct command is selected for each platform without | ||
| * spawning a subprocess. Follows the conventions in | ||
| * tests/unit/cli-dashboard-port.test.ts. | ||
| */ | ||
|
|
||
| type Platform = "darwin" | "win32" | string; | ||
|
|
||
| function openFallbackCommand(platform: Platform, url: string): { cmd: string; args: string[] } { | ||
| let cmd: string; | ||
| let args: string[]; | ||
|
|
||
| if (platform === "darwin") { | ||
| cmd = "open"; | ||
| args = [url]; | ||
| } else if (platform === "win32") { | ||
| cmd = "rundll32"; | ||
| args = ["url.dll,FileProtocolHandler", url]; | ||
| } else { | ||
| cmd = "xdg-open"; | ||
| args = [url]; | ||
| } | ||
|
|
||
| return { cmd, args }; | ||
| } | ||
|
|
||
| test("openFallback: darwin uses 'open' command", () => { | ||
| const { cmd, args } = openFallbackCommand("darwin", "http://localhost:20128"); | ||
| assert.equal(cmd, "open"); | ||
| assert.deepEqual(args, ["http://localhost:20128"]); | ||
| }); | ||
|
|
||
| test("openFallback: win32 uses 'rundll32 url.dll,FileProtocolHandler' instead of cmd.exe", () => { | ||
| const { cmd, args } = openFallbackCommand("win32", "http://localhost:20128"); | ||
| assert.equal(cmd, "rundll32"); | ||
| assert.deepEqual(args, ["url.dll,FileProtocolHandler", "http://localhost:20128"]); | ||
| }); | ||
|
|
||
| test("openFallback: win32 does NOT use cmd /c start", () => { | ||
| const { cmd } = openFallbackCommand("win32", "http://localhost:20128"); | ||
| assert.notEqual(cmd, "cmd"); | ||
| }); | ||
|
|
||
| test("openFallback: linux/other uses 'xdg-open' command", () => { | ||
| const { cmd, args } = openFallbackCommand("linux", "http://localhost:20128"); | ||
| assert.equal(cmd, "xdg-open"); | ||
| assert.deepEqual(args, ["http://localhost:20128"]); | ||
| }); | ||
|
|
||
| test("openFallback: unknown platform falls back to xdg-open", () => { | ||
| const { cmd, args } = openFallbackCommand("aix", "http://localhost:20128"); | ||
| assert.equal(cmd, "xdg-open"); | ||
| assert.deepEqual(args, ["http://localhost:20128"]); | ||
| }); | ||
|
|
||
| test("openFallback: URL with special characters is passed through verbatim", () => { | ||
| const url = "http://localhost:20128/dashboard?q=test&filter=a+b"; | ||
| const { cmd, args } = openFallbackCommand("win32", url); | ||
| assert.equal(cmd, "rundll32"); | ||
| assert.equal(args[1], url); | ||
| }); | ||
|
|
||
| test("openFallback: execFile call in source does NOT reference cmd.exe on win32", async () => { | ||
| const fs = await import("node:fs"); | ||
| const path = await import("node:path"); | ||
| const source = fs.readFileSync( | ||
| path.resolve(import.meta.dirname, "../../bin/cli/commands/dashboard.mjs"), | ||
| "utf-8", | ||
| ); | ||
|
|
||
| // The win32 branch must not contain "cmd" as the command | ||
| const win32Branch = source.match(/platform === "win32"[\s\S]*?\}[\s\n]*else\s*\{/); | ||
| assert.ok(win32Branch, "should have a win32 branch in openFallback"); | ||
| assert.doesNotMatch(win32Branch[0], /"cmd"/, "win32 branch must not set cmd to 'cmd'"); | ||
| assert.match(win32Branch[0], /rundll32/, "win32 branch must use rundll32"); | ||
| }); |
There was a problem hiding this comment.
Duplicating production logic inside test files is a testing anti-pattern. Currently, the test file defines its own copy of openFallbackCommand, meaning the actual production code in bin/cli/commands/dashboard.mjs is never executed or verified during tests. This can lead to false positives (tests passing while production code is broken) and prevents the test suite from contributing to the required 75% coverage threshold for the actual production code.
By importing the newly exported getFallbackCommand helper from the production file, we can test the actual implementation directly and ensure robust coverage.
import test from "node:test";
import assert from "node:assert/strict";
import { getFallbackCommand as openFallbackCommand } from "../../bin/cli/commands/dashboard.mjs";
test("openFallback: darwin uses 'open' command", () => {
const { cmd, args } = openFallbackCommand("darwin", "http://localhost:20128");
assert.equal(cmd, "open");
assert.deepEqual(args, ["http://localhost:20128"]);
});
test("openFallback: win32 uses 'explorer' instead of cmd.exe or rundll32", () => {
const { cmd, args } = openFallbackCommand("win32", "http://localhost:20128");
assert.equal(cmd, "explorer");
assert.deepEqual(args, ["http://localhost:20128"]);
});
test("openFallback: win32 does NOT use cmd /c start", () => {
const { cmd } = openFallbackCommand("win32", "http://localhost:20128");
assert.notEqual(cmd, "cmd");
});
test("openFallback: linux/other uses 'xdg-open' command", () => {
const { cmd, args } = openFallbackCommand("linux", "http://localhost:20128");
assert.equal(cmd, "xdg-open");
assert.deepEqual(args, ["http://localhost:20128"]);
});
test("openFallback: unknown platform falls back to xdg-open", () => {
const { cmd, args } = openFallbackCommand("aix", "http://localhost:20128");
assert.equal(cmd, "xdg-open");
assert.deepEqual(args, ["http://localhost:20128"]);
});
test("openFallback: URL with special characters is passed through verbatim", () => {
const url = "http://localhost:20128/dashboard?q=test&filter=a+b";
const { cmd, args } = openFallbackCommand("win32", url);
assert.equal(cmd, "explorer");
assert.equal(args[0], url);
});
test("openFallback: execFile call in source does NOT reference cmd.exe on win32", async () => {
const fs = await import("node:fs");
const path = await import("node:path");
const source = fs.readFileSync(
path.resolve(import.meta.dirname, "../../bin/cli/commands/dashboard.mjs"),
"utf-8",
);
// The win32 branch must not contain "cmd" as the command
const win32Branch = source.match(/platform === "win32"[\s\S]*?\}[\s\n]*else\s*\{/);
assert.ok(win32Branch, "should have a win32 branch in openFallback");
assert.doesNotMatch(win32Branch[0], /"cmd"/, "win32 branch must not set cmd to 'cmd'");
assert.match(win32Branch[0], /explorer/, "win32 branch must use explorer");
});References
- Always include tests when changing production code and maintain coverage >= 75%. Testing the actual production code directly ensures that coverage is correctly measured and that production changes are genuinely validated.
…k in dashboard command Extract resolveOpenCommand(platform, url) as an exported pure function so tests import the actual production code instead of duplicating logic. The openFallback function in bin/cli/commands/dashboard.mjs used cmd /c start to open the dashboard URL on Windows, spawning an unnecessary cmd.exe process. Replaces with rundll32 url.dll,FileProtocolHandler which opens the URL directly through the Windows shell handler API without any shell wrapper. Tests: 5 tests importing the actual resolveOpenCommand function, covering all platform branches (darwin, win32, linux) and URL pass-through. Changelog fragment included.
933f5f7 to
a0a5e40
Compare
|
Nice, focused fix — verified locally: all 5 tests in One small nit for a follow-up (non-blocking, won't hold up the merge): Thanks for the clean extraction and the consistency with the #7841 machineId fix — merging this as-is. |
|
Reconciled this against your #7841, since both attack the same "stop spawning We're deliberately not asking you to consolidate them into a shared helper — both proceed independently as-is, in either order. |
7d82b72
into
cheaper-inference:release/v3.8.49
|
Merged — thanks @tientien17! rundll32 via execFile is exactly the shell-free shape we want on Windows. Validated in the local merge-train (14-PR combined tree, full unit suite + vitest green) on tomni-proxmox-113 @ 0fda37bb4fdb26ba1345fd7b4cac44fbe235cda4, then squash-merged. |
…k in dashboard command (cheaper-inference#7844) * fix(cli): use rundll32 instead of cmd.exe for Windows browser fallback in dashboard command Extract resolveOpenCommand(platform, url) as an exported pure function so tests import the actual production code instead of duplicating logic. The openFallback function in bin/cli/commands/dashboard.mjs used cmd /c start to open the dashboard URL on Windows, spawning an unnecessary cmd.exe process. Replaces with rundll32 url.dll,FileProtocolHandler which opens the URL directly through the Windows shell handler API without any shell wrapper. Tests: 5 tests importing the actual resolveOpenCommand function, covering all platform branches (darwin, win32, linux) and URL pass-through. Changelog fragment included. * chore: rename changelog fragment 7842->7844 to match actual PR number --------- Co-authored-by: tientien17 <tientien17@users.noreply.github.com>
…k in dashboard command (cheaper-inference#7844) * fix(cli): use rundll32 instead of cmd.exe for Windows browser fallback in dashboard command Extract resolveOpenCommand(platform, url) as an exported pure function so tests import the actual production code instead of duplicating logic. The openFallback function in bin/cli/commands/dashboard.mjs used cmd /c start to open the dashboard URL on Windows, spawning an unnecessary cmd.exe process. Replaces with rundll32 url.dll,FileProtocolHandler which opens the URL directly through the Windows shell handler API without any shell wrapper. Tests: 5 tests importing the actual resolveOpenCommand function, covering all platform branches (darwin, win32, linux) and URL pass-through. Changelog fragment included. * chore: rename changelog fragment 7842->7844 to match actual PR number --------- Co-authored-by: tientien17 <tientien17@users.noreply.github.com>
Summary
The \openFallback\ function in \�in/cli/commands/dashboard.mjs\ used \cmd /c start\ to open the dashboard URL on Windows, spawning an unnecessary \cmd.exe\ process. This replaces it with
undll32 url.dll,FileProtocolHandler\ which opens the URL directly through the Windows shell handler API without any shell wrapper — consistent with the machineId fix in PR #7841.
This affects the fallback path only (when the \open\ npm package fails or is unavailable). The \open\ v11 package already uses PowerShell on Windows, not cmd.exe.
Related Issues
Validation
px tsx --test tests/unit/cli-dashboard-open-fallback.test.ts\ — 7/7 tests pass
Tests Added Or Updated
Reviewer Notes
Small 2-line production change behind the \openFallback\ catch block. No user-facing behavior change on macOS/Linux or when the primary \open\ package succeeds.