refactor: unify CLI flag parsing across interactive and headless (#1137)

Co-authored-by: cpacker <packercharles@gmail.com>

src/tests/cli/args.test.ts

@@ -0,0 +1,133 @@
+import { describe, expect, test } from "bun:test";
+import {
+  CLI_FLAG_CATALOG,
+  CLI_OPTIONS,
+  parseCliArgs,
+  preprocessCliArgs,
+  renderCliOptionsHelp,
+} from "../../cli/args";
+
+describe("shared CLI arg schema", () => {
+  test("catalog is the single source of truth for parser mapping and mode support", () => {
+    const catalogKeys = Object.keys(CLI_FLAG_CATALOG).sort();
+    const optionKeys = Object.keys(CLI_OPTIONS).sort();
+    expect(optionKeys).toEqual(catalogKeys);
+
+    const validModes = new Set(["interactive", "headless", "both"]);
+    const validTypes = new Set(["boolean", "string"]);
+
+    for (const [flagName, definition] of Object.entries(CLI_FLAG_CATALOG)) {
+      expect(validModes.has(definition.mode)).toBe(true);
+      expect(validTypes.has(definition.parser.type)).toBe(true);
+      expect(CLI_OPTIONS[flagName]).toEqual(definition.parser);
+    }
+  });
+
+  test("mode lookups include shared flags and exclude opposite-mode-only flags", () => {
+    const getFlagsForMode = (mode: "headless" | "interactive") =>
+      Object.entries(CLI_FLAG_CATALOG)
+        .filter(
+          ([, definition]) =>
+            definition.mode === "both" || definition.mode === mode,
+        )
+        .map(([name]) => name);
+    const headlessFlags = getFlagsForMode("headless");
+    const interactiveFlags = getFlagsForMode("interactive");
+
+    expect(headlessFlags).toContain("memfs-startup");
+    expect(headlessFlags).not.toContain("resume");
+    expect(interactiveFlags).toContain("resume");
+    expect(interactiveFlags).not.toContain("memfs-startup");
+    expect(headlessFlags).toContain("agent");
+    expect(interactiveFlags).toContain("agent");
+  });
+
+  test("rendered OPTIONS help is generated from catalog metadata", () => {
+    const help = renderCliOptionsHelp();
+    expect(help).toContain("-h, --help");
+    expect(help).toContain("-c, --continue");
+    expect(help).toContain("--memfs-startup <m>");
+    expect(help).toContain("Default: text");
+    expect(help).not.toContain("--run");
+
+    for (const [flagName, definition] of Object.entries(
+      CLI_FLAG_CATALOG,
+    ) as Array<[string, { help?: unknown }]>) {
+      if (!definition.help) continue;
+      expect(help).toContain(`--${flagName}`);
+    }
+  });
+
+  test("normalizes --conv alias to --conversation", () => {
+    const parsed = parseCliArgs(
+      preprocessCliArgs([
+        "node",
+        "script",
+        "--conv",
+        "conv-123",
+        "-p",
+        "hello",
+      ]),
+      true,
+    );
+    expect(parsed.values.conversation).toBe("conv-123");
+    expect(parsed.positionals.slice(2).join(" ")).toBe("hello");
+  });
+
+  test("recognizes headless-specific startup flags in strict mode", () => {
+    const parsed = parseCliArgs(
+      preprocessCliArgs([
+        "node",
+        "script",
+        "-p",
+        "hello",
+        "--memfs-startup",
+        "background",
+        "--pre-load-skills",
+        "skill-a,skill-b",
+        "--max-turns",
+        "3",
+        "--block-value",
+        "persona=hello",
+      ]),
+      true,
+    );
+    expect(parsed.values["memfs-startup"]).toBe("background");
+    expect(parsed.values["pre-load-skills"]).toBe("skill-a,skill-b");
+    expect(parsed.values["max-turns"]).toBe("3");
+    expect(parsed.values["block-value"]).toEqual(["persona=hello"]);
+  });
+
+  test("treats --import argument as a flag value, not prompt text", () => {
+    const parsed = parseCliArgs(
+      preprocessCliArgs([
+        "node",
+        "script",
+        "-p",
+        "hello",
+        "--import",
+        "@author/agent",
+      ]),
+      true,
+    );
+    expect(parsed.values.import).toBe("@author/agent");
+    expect(parsed.positionals.slice(2).join(" ")).toBe("hello");
+  });
+
+  test("supports short aliases used by headless and interactive modes", () => {
+    const parsed = parseCliArgs(
+      preprocessCliArgs([
+        "node",
+        "script",
+        "-p",
+        "hello",
+        "-c",
+        "-C",
+        "conv-123",
+      ]),
+      true,
+    );
+    expect(parsed.values.continue).toBe(true);
+    expect(parsed.values.conversation).toBe("conv-123");
+  });
+});

src/tests/cli/flag-utils.test.ts

@@ -0,0 +1,51 @@
+import { describe, expect, test } from "bun:test";
+import {
+  parseCsvListFlag,
+  parseJsonArrayFlag,
+  parsePositiveIntFlag,
+  resolveImportFlagAlias,
+} from "../../cli/flagUtils";
+
+describe("flag utils", () => {
+  test("parseCsvListFlag handles undefined and none", () => {
+    expect(parseCsvListFlag(undefined)).toBeUndefined();
+    expect(parseCsvListFlag("none")).toEqual([]);
+    expect(parseCsvListFlag("a, b ,c")).toEqual(["a", "b", "c"]);
+  });
+
+  test("resolveImportFlagAlias prefers --import", () => {
+    expect(
+      resolveImportFlagAlias({
+        importFlagValue: "@author/agent",
+        fromAfFlagValue: "path.af",
+      }),
+    ).toBe("@author/agent");
+    expect(
+      resolveImportFlagAlias({
+        importFlagValue: undefined,
+        fromAfFlagValue: "path.af",
+      }),
+    ).toBe("path.af");
+  });
+
+  test("parsePositiveIntFlag validates positive integers", () => {
+    expect(
+      parsePositiveIntFlag({
+        rawValue: "3",
+        flagName: "max-turns",
+      }),
+    ).toBe(3);
+    expect(() =>
+      parsePositiveIntFlag({ rawValue: "0", flagName: "max-turns" }),
+    ).toThrow("--max-turns must be a positive integer");
+  });
+
+  test("parseJsonArrayFlag parses arrays and rejects non-arrays", () => {
+    expect(
+      parseJsonArrayFlag('[{"label":"persona"}]', "memory-blocks"),
+    ).toEqual([{ label: "persona" }]);
+    expect(() =>
+      parseJsonArrayFlag('{"label":"persona"}', "memory-blocks"),
+    ).toThrow("memory-blocks must be a JSON array");
+  });
+});

src/tests/cli/startup-flag-validation.test.ts

@@ -0,0 +1,60 @@
+import { describe, expect, test } from "bun:test";
+import {
+  validateConversationDefaultRequiresAgent,
+  validateFlagConflicts,
+  validateRegistryHandleOrThrow,
+} from "../../cli/startupFlagValidation";
+
+describe("startup flag validation helpers", () => {
+  test("conversation default requires agent unless new-agent is set", () => {
+    expect(() =>
+      validateConversationDefaultRequiresAgent({
+        specifiedConversationId: "default",
+        specifiedAgentId: null,
+        forceNew: false,
+      }),
+    ).toThrow("--conv default requires --agent <agent-id>");
+
+    expect(() =>
+      validateConversationDefaultRequiresAgent({
+        specifiedConversationId: "default",
+        specifiedAgentId: "agent-123",
+        forceNew: false,
+      }),
+    ).not.toThrow();
+  });
+
+  test("conflict helpers throw the first matching conflict", () => {
+    expect(() =>
+      validateFlagConflicts({
+        guard: true,
+        checks: [
+          { when: true, message: "conversation conflict" },
+          { when: true, message: "should not hit second" },
+        ],
+      }),
+    ).toThrow("conversation conflict");
+
+    expect(() =>
+      validateFlagConflicts({
+        guard: true,
+        checks: [{ when: true, message: "new conflict" }],
+      }),
+    ).toThrow("new conflict");
+
+    expect(() =>
+      validateFlagConflicts({
+        guard: "@author/agent",
+        checks: [{ when: true, message: "import conflict" }],
+      }),
+    ).toThrow("import conflict");
+  });
+
+  test("registry handle validator accepts valid handles and rejects invalid ones", () => {
+    expect(() => validateRegistryHandleOrThrow("@author/agent")).not.toThrow();
+    expect(() => validateRegistryHandleOrThrow("author/agent")).not.toThrow();
+    expect(() => validateRegistryHandleOrThrow("@author")).toThrow(
+      'Invalid registry handle "@author"',
+    );
+  });
+});

src/tests/startup-flow.test.ts

@@ -114,6 +114,19 @@ describe("Startup Flow - Flag Conflicts", () => {
     );
   });
 
+  test("--conversation conflicts with legacy --from-af using canonical --import error text", async () => {
+    const result = await runCli(
+      ["--conversation", "conv-123", "--from-af", "test.af"],
+      { expectExit: 1 },
+    );
+    expect(result.stderr).toContain(
+      "--conversation cannot be used with --import",
+    );
+    expect(result.stderr).not.toContain(
+      "--conversation cannot be used with --from-af",
+    );
+  });
+
   test("--conversation conflicts with --name", async () => {
     const result = await runCli(
       ["--conversation", "conv-123", "--name", "MyAgent"],
@@ -123,6 +136,14 @@ describe("Startup Flow - Flag Conflicts", () => {
       "--conversation cannot be used with --name",
     );
   });
+
+  test("--import conflicts with --name (including legacy --from-af alias)", async () => {
+    const result = await runCli(["--from-af", "test.af", "--name", "MyAgent"], {
+      expectExit: 1,
+    });
+    expect(result.stderr).toContain("--import cannot be used with --name");
+    expect(result.stderr).not.toContain("--from-af cannot be used with --name");
+  });
 });
 
 describe("Startup Flow - Smoke", () => {
@@ -130,7 +151,20 @@ describe("Startup Flow - Smoke", () => {
     const result = await runCli(["--name", "MyAgent", "--new-agent"], {
       expectExit: 1,
     });
-    expect(result.stderr).toContain("--name cannot be used with --new");
+    expect(result.stderr).toContain("--name cannot be used with --new-agent");
+  });
+
+  test("--new + --name does not conflict (new conversation on named agent)", async () => {
+    const result = await runCli(
+      ["-p", "Say OK", "--new", "--name", "NonExistentAgent999"],
+      { expectExit: 1 },
+    );
+    // Should get past flag validation regardless of whether credentials exist.
+    expect(result.stderr).not.toContain("cannot be used with");
+    expect(
+      result.stderr.includes("NonExistentAgent999") ||
+        result.stderr.includes("Missing LETTA_API_KEY"),
+    ).toBe(true);
   });
 
   test("--new-agent headless parses and reaches credential check", async () => {
@@ -151,4 +185,57 @@ describe("Startup Flow - Smoke", () => {
     expect(result.stderr).toContain("Missing LETTA_API_KEY");
     expect(result.stderr).not.toContain("Invalid toolset");
   });
+
+  test("--memfs-startup is accepted for headless startup", async () => {
+    const result = await runCli(
+      ["--new-agent", "-p", "Say OK", "--memfs-startup", "background"],
+      {
+        expectExit: 1,
+      },
+    );
+    expect(result.stderr).toContain("Missing LETTA_API_KEY");
+    expect(result.stderr).not.toContain("Unknown option '--memfs-startup'");
+  });
+
+  test("-c alias for --continue is accepted", async () => {
+    const result = await runCli(["-p", "Say OK", "-c"], {
+      expectExit: 1,
+    });
+    expect(result.stderr).toContain("Missing LETTA_API_KEY");
+    expect(result.stderr).not.toContain("Unknown option '-c'");
+  });
+
+  test("-C alias for --conversation is accepted", async () => {
+    const result = await runCli(["-p", "Say OK", "-C", "conv-123"], {
+      expectExit: 1,
+    });
+    expect(result.stderr).toContain("Missing LETTA_API_KEY");
+    expect(result.stderr).not.toContain("Unknown option '-C'");
+  });
+
+  test("--import handle is accepted in headless mode", async () => {
+    const result = await runCli(["--import", "@author/agent", "-p", "Say OK"], {
+      expectExit: 1,
+    });
+    expect(result.stderr).toContain("Missing LETTA_API_KEY");
+    expect(result.stderr).not.toContain("Invalid registry handle");
+  });
+
+  test("--max-turns and --pre-load-skills are accepted in headless mode", async () => {
+    const result = await runCli(
+      [
+        "--new-agent",
+        "-p",
+        "Say OK",
+        "--max-turns",
+        "2",
+        "--pre-load-skills",
+        "foo,bar",
+      ],
+      { expectExit: 1 },
+    );
+    expect(result.stderr).toContain("Missing LETTA_API_KEY");
+    expect(result.stderr).not.toContain("Unknown option '--max-turns'");
+    expect(result.stderr).not.toContain("Unknown option '--pre-load-skills'");
+  });
 });