fix(cli): address PR #983 review feedback

- play.ts: move --remote-debugging-port parse+deps validation before any
  server setup so an invalid value exits cleanly instead of leaking a
  listening socket (the original bug — server printed 'Player running'
  and 'Press Ctrl+C to stop' before failing).
- Extract validateRemoteDebuggingPortDeps() in openBrowser.ts to keep
  preview.ts and play.ts in sync instead of copy-pasting the dep
  checks.
- Narrow parseRemoteDebuggingPort param to string | undefined; drop the
  dead null branch and the redundant String() / Number.isInteger() now
  that the regex already constrains the input.
- buildBrowserArgs: omit --remote-debugging-port when userDataDir is
  missing so a CDP endpoint cannot leak into the user's main profile
  even if a caller bypasses the CLI validation layer.
- Replace the duplicated buildBrowserArgs case with one that proves
  this defense-in-depth behaviour; add unit tests for
  validateRemoteDebuggingPortDeps.
- Drop the heavy JSDoc on parseRemoteDebuggingPort to match the file's
  surrounding style.
- Both commands: align --remote-debugging-port description (it now
  matches the actual 'requires --browser-path and --user-data-dir'
  contract) and add a CDP example to the --help output.
This commit is contained in:
AnoKno
2026-05-25 15:38:41 -04:00
committed by Miguel Ángel
parent 3902a9a82b
commit 0ea8aa4ffa
4 changed files with 137 additions and 69 deletions
+31 -23
View File
@@ -8,12 +8,20 @@ export const examples: Example[] = [
["Use a custom port", "hyperframes play --port 8080"],
["Start without opening the browser", "hyperframes play --no-open"],
["Open with a specific browser", "hyperframes play --browser-path /usr/bin/chromium"],
[
"Open with CDP enabled (requires browser path + isolated profile)",
"hyperframes play --browser-path /usr/bin/chromium --user-data-dir /tmp/hf-profile --remote-debugging-port 9222",
],
];
import { resolve, dirname } from "node:path";
import * as clack from "@clack/prompts";
import { c } from "../ui/colors.js";
import { resolveProject } from "../utils/project.js";
import { openBrowser, parseRemoteDebuggingPort } from "../utils/openBrowser.js";
import {
openBrowser,
parseRemoteDebuggingPort,
validateRemoteDebuggingPortDeps,
} from "../utils/openBrowser.js";
export default defineCommand({
meta: { name: "play", description: "Play a composition in a lightweight browser player" },
@@ -48,18 +56,28 @@ export default defineCommand({
process.exitCode = 1;
return;
}
// Validation: --remote-debugging-port requires --browser-path and --user-data-dir
if (args["remote-debugging-port"]) {
if (!args["browser-path"]) {
clack.log.error("--remote-debugging-port requires --browser-path");
process.exitCode = 1;
return;
}
if (!args["user-data-dir"]) {
clack.log.error("--remote-debugging-port requires --user-data-dir");
process.exitCode = 1;
return;
}
// Validation: --remote-debugging-port deps
const depsError = validateRemoteDebuggingPortDeps({
browserPath: args["browser-path"] as string | undefined,
userDataDir: args["user-data-dir"] as string | undefined,
remoteDebuggingPort: args["remote-debugging-port"] as string | undefined,
});
if (depsError) {
clack.log.error(depsError);
process.exitCode = 1;
return;
}
// Parse --remote-debugging-port before any server setup so an invalid value
// exits cleanly instead of leaving an orphan listening socket behind.
let remoteDebuggingPort: number | undefined;
try {
remoteDebuggingPort = parseRemoteDebuggingPort(
args["remote-debugging-port"] as string | undefined,
);
} catch (err) {
clack.log.error((err as Error).message);
process.exitCode = 1;
return;
}
// Resolve runtime path — same logic as studioServer.ts
@@ -185,16 +203,6 @@ export default defineCommand({
console.log();
console.log(` ${c.dim("Press Ctrl+C to stop")}`);
console.log();
let remoteDebuggingPort: number | undefined;
if (args["remote-debugging-port"]) {
try {
remoteDebuggingPort = parseRemoteDebuggingPort(args["remote-debugging-port"]);
} catch (err) {
clack.log.error((err as Error).message);
process.exitCode = 1;
return;
}
}
if (args.open) {
void openBrowser(url, {
+27 -21
View File
@@ -9,6 +9,10 @@ export const examples: Example[] = [
["Force a new server even if one is already running", "hyperframes preview --force-new"],
["Start without opening the browser", "hyperframes preview --no-open"],
["Open with a specific browser", "hyperframes preview --browser-path /usr/bin/chromium"],
[
"Open with CDP enabled (requires browser path + isolated profile)",
"hyperframes preview --browser-path /usr/bin/chromium --user-data-dir /tmp/hf-profile --remote-debugging-port 9222",
],
["List all active preview servers", "hyperframes preview --list"],
["Kill all active preview servers", "hyperframes preview --kill-all"],
];
@@ -19,7 +23,11 @@ import { createRequire } from "node:module";
import * as clack from "@clack/prompts";
import { c } from "../ui/colors.js";
import { isDevMode } from "../utils/env.js";
import { openBrowser, parseRemoteDebuggingPort } from "../utils/openBrowser.js";
import {
openBrowser,
parseRemoteDebuggingPort,
validateRemoteDebuggingPortDeps,
} from "../utils/openBrowser.js";
import { lintProject } from "../utils/lintProject.js";
import { formatLintFindings } from "../utils/lintFormat.js";
import {
@@ -135,32 +143,30 @@ export default defineCommand({
process.exitCode = 1;
return;
}
// Validation: --remote-debugging-port requires --browser-path and --user-data-dir
if (args["remote-debugging-port"]) {
if (!args["browser-path"]) {
clack.log.error("--remote-debugging-port requires --browser-path");
process.exitCode = 1;
return;
}
if (!args["user-data-dir"]) {
clack.log.error("--remote-debugging-port requires --user-data-dir");
process.exitCode = 1;
return;
}
// Validation: --remote-debugging-port deps
const depsError = validateRemoteDebuggingPortDeps({
browserPath: args["browser-path"] as string | undefined,
userDataDir: args["user-data-dir"] as string | undefined,
remoteDebuggingPort: args["remote-debugging-port"] as string | undefined,
});
if (depsError) {
clack.log.error(depsError);
process.exitCode = 1;
return;
}
const noOpen = !args.open;
const browserPath = args["browser-path"] as string | undefined;
const userDataDir = args["user-data-dir"] as string | undefined;
let remoteDebuggingPort: number | undefined;
if (args["remote-debugging-port"]) {
try {
remoteDebuggingPort = parseRemoteDebuggingPort(args["remote-debugging-port"]);
} catch (err) {
clack.log.error((err as Error).message);
process.exitCode = 1;
return;
}
try {
remoteDebuggingPort = parseRemoteDebuggingPort(
args["remote-debugging-port"] as string | undefined,
);
} catch (err) {
clack.log.error((err as Error).message);
process.exitCode = 1;
return;
}
if (isDevMode()) {
+52 -8
View File
@@ -1,5 +1,9 @@
import { describe, it, expect } from "vitest";
import { buildBrowserArgs, parseRemoteDebuggingPort } from "./openBrowser.js";
import {
buildBrowserArgs,
parseRemoteDebuggingPort,
validateRemoteDebuggingPortDeps,
} from "./openBrowser.js";
describe("buildBrowserArgs", () => {
it("returns only the URL when no options are given", () => {
@@ -38,18 +42,16 @@ describe("buildBrowserArgs", () => {
).toEqual(["--user-data-dir=C:\\Documents and Settings\\profile", "http://localhost:3002"]);
});
it("prepends --remote-debugging-port before the URL", () => {
it("omits --remote-debugging-port when userDataDir is missing (defense in depth)", () => {
// The CLI validation layer rejects this combination upstream, but
// buildBrowserArgs must not leak a CDP endpoint into the user's main
// profile even if a caller bypasses that check.
expect(
buildBrowserArgs("http://localhost:3002", {
browserPath: "/usr/bin/chromium",
userDataDir: "/tmp/hf-profile",
remoteDebuggingPort: 9222,
}),
).toEqual([
"--user-data-dir=/tmp/hf-profile",
"--remote-debugging-port=9222",
"http://localhost:3002",
]);
).toEqual(["http://localhost:3002"]);
});
it("includes all flags together", () => {
@@ -114,3 +116,45 @@ describe("parseRemoteDebuggingPort", () => {
expect(() => parseRemoteDebuggingPort("22.5")).toThrow();
});
});
describe("validateRemoteDebuggingPortDeps", () => {
it("returns null when --remote-debugging-port is not set", () => {
expect(validateRemoteDebuggingPortDeps({})).toBeNull();
});
it("returns null when all required flags are present", () => {
expect(
validateRemoteDebuggingPortDeps({
browserPath: "/usr/bin/chromium",
userDataDir: "/tmp/hf-profile",
remoteDebuggingPort: "9222",
}),
).toBeNull();
});
it("requires --browser-path when --remote-debugging-port is set", () => {
expect(
validateRemoteDebuggingPortDeps({
userDataDir: "/tmp/hf-profile",
remoteDebuggingPort: "9222",
}),
).toBe("--remote-debugging-port requires --browser-path");
});
it("requires --user-data-dir when --remote-debugging-port is set", () => {
expect(
validateRemoteDebuggingPortDeps({
browserPath: "/usr/bin/chromium",
remoteDebuggingPort: "9222",
}),
).toBe("--remote-debugging-port requires --user-data-dir");
});
it("reports --browser-path first when both deps are missing", () => {
expect(
validateRemoteDebuggingPortDeps({
remoteDebuggingPort: "9222",
}),
).toBe("--remote-debugging-port requires --browser-path");
});
});
+27 -17
View File
@@ -6,29 +6,35 @@ export interface OpenBrowserOptions {
remoteDebuggingPort?: number;
}
/**
* Validate and parse a --remote-debugging-port value.
* Returns the port number or undefined if not provided.
* Throws if the value is not a valid integer in 1..65535.
*/
export function parseRemoteDebuggingPort(value: unknown): number | undefined {
if (value === undefined || value === null || value === "") return undefined;
const text = String(value);
if (!/^\d+$/.test(text)) {
export function parseRemoteDebuggingPort(value: string | undefined): number | undefined {
if (value === undefined || value === "") return undefined;
if (!/^\d+$/.test(value)) {
throw new Error("--remote-debugging-port must be an integer between 1 and 65535");
}
const port = Number(text);
if (!Number.isInteger(port) || port < 1 || port > 65535) {
const port = Number(value);
if (port < 1 || port > 65535) {
throw new Error("--remote-debugging-port must be an integer between 1 and 65535");
}
return port;
}
export interface RemoteDebuggingPortDeps {
browserPath?: string;
userDataDir?: string;
remoteDebuggingPort?: string;
}
/**
* Returns an error message if --remote-debugging-port is set without its required
* dependencies (--browser-path and --user-data-dir), or null if everything is OK.
*/
export function validateRemoteDebuggingPortDeps(deps: RemoteDebuggingPortDeps): string | null {
if (!deps.remoteDebuggingPort) return null;
if (!deps.browserPath) return "--remote-debugging-port requires --browser-path";
if (!deps.userDataDir) return "--remote-debugging-port requires --user-data-dir";
return null;
}
/**
* Build the argument list for spawning a browser process.
*
@@ -39,7 +45,11 @@ export function buildBrowserArgs(url: string, options: OpenBrowserOptions): stri
if (options.userDataDir) {
args.push(`--user-data-dir=${options.userDataDir}`);
}
if (options.remoteDebuggingPort !== undefined) {
// Defense-in-depth: only emit --remote-debugging-port when paired with an
// isolated --user-data-dir. Without an isolated profile the CDP endpoint
// would expose the user's main browser session, which is the whole reason
// the CLI validation layer requires both flags together.
if (options.remoteDebuggingPort !== undefined && options.userDataDir) {
args.push(`--remote-debugging-port=${options.remoteDebuggingPort}`);
}
args.push(url);