fix(cli): address PR #983 review feedback

AnoKno · AnoKno · commit 4affe2ba5529 · 2026-05-21T02:25:41.000+09:00
- 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.
diff --git a/packages/cli/src/commands/play.ts b/packages/cli/src/commands/play.ts
@@ -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, {
diff --git a/packages/cli/src/commands/preview.ts b/packages/cli/src/commands/preview.ts
@@ -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 {
@@ -126,32 +134,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()) {
diff --git a/packages/cli/src/utils/openBrowser.test.ts b/packages/cli/src/utils/openBrowser.test.ts
@@ -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");
+  });
+});
diff --git a/packages/cli/src/utils/openBrowser.ts b/packages/cli/src/utils/openBrowser.ts
@@ -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);
