Improve dice notation validation and error handling in CLI (#8)

* feat(cli): add robust error handling for invalid dice notation input

- ParseDiceNotation now validates each part of the input string and collects warnings for:
  - Non-numeric dice counts
  - Non-integer dice counts (decimals, commas)
  - Missing dice color
  - Unrecognized dice colors
- Warnings are printed to stderr to inform users of invalid parts without stopping execution.
- The main CLI function checks if any valid dice were parsed; if none, it prints an error message and exits with code 1.
- Extensive tests added to cover various invalid input scenarios and ensure warnings and errors are handled correctly.
- package-lock.json updated to include CLI binary and node engine requirement.

This improves user experience by providing clear feedback on input errors and preventing silent failures or confusing results.

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

* test(cli): refactor console.error mocks and improve error message checks

- Changed console.error mock to be recreated fresh in beforeEach and restored in afterEach
- Updated error message assertions to check for specific messages in multiple calls
- Improved clarity and reliability of CLI error handling tests

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

---------

Co-authored-by: terragon-labs[bot] <terragon-labs[bot]@users.noreply.github.com>

Author: jziggas

package-lock.json

@@ -15,12 +15,41 @@ export function parseDiceNotation(input: string): DicePool {
     forceDice: 0,
   };
 
-  const parts = input.toLowerCase().trim().split(" ");
+  const warnings: string[] = [];
+  const parts = input
+    .toLowerCase()
+    .trim()
+    .split(" ")
+    .filter((p) => p.length > 0);
 
   for (const part of parts) {
     const count = parseInt(part);
+
+    // Check if parseInt returned NaN
+    if (isNaN(count)) {
+      warnings.push(`Invalid dice notation: "${part}" - number not found`);
+      continue;
+    }
+
+    // Check for non-integer values
+    if (part.includes(".") || part.includes(",")) {
+      warnings.push(
+        `Invalid dice notation: "${part}" - dice count must be a whole number`,
+      );
+      continue;
+    }
+
     const color = part.slice(String(count).length).toLowerCase();
 
+    // Skip if no color specified
+    if (!color) {
+      warnings.push(
+        `Invalid dice notation: "${part}" - no dice color specified`,
+      );
+      continue;
+    }
+
+    let recognized = true;
     switch (color) {
       // y/pro = Yellow / Proficiency
       case "y":
@@ -77,9 +106,17 @@ export function parseDiceNotation(input: string): DicePool {
       case "f":
         pool.forceDice = count;
         break;
+      default:
+        recognized = false;
+        warnings.push(`Invalid dice color: "${color}" in "${part}"`);
     }
   }
 
+  // Print warnings to stderr
+  if (warnings.length > 0) {
+    warnings.forEach((warning) => console.error(`Warning: ${warning}`));
+  }
+
   return pool;
 }
 
@@ -133,6 +170,18 @@ export const main = () => {
   }
 
   const pool = parseDiceNotation(diceNotation);
+
+  // Check if the pool is empty (all zeros) which might indicate invalid input
+  const hasAnyDice = Object.values(pool).some((count) => count > 0);
+  if (!hasAnyDice) {
+    console.error("\nError: No valid dice found in the notation.");
+    console.error(
+      "Please check your input and ensure it follows the format: <count><color>",
+    );
+    console.error("Example: '2y 1g' for 2 yellow and 1 green dice\n");
+    process.exit(1);
+  }
+
   const result = roll(pool, { hints: showHints });
   console.log(formatResult(result));
 };

src/cli.ts

@@ -294,6 +294,16 @@ describe("CLI Functions", () => {
 });
 
 describe("Error handling", () => {
+  let consoleErrorSpy: jest.SpyInstance;
+
+  beforeEach(() => {
+    consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {});
+  });
+
+  afterEach(() => {
+    consoleErrorSpy.mockRestore();
+  });
+
   test("handles invalid dice notation", () => {
     expect(parseDiceNotation("invalid")).toEqual({
       boostDice: 0,
@@ -304,7 +314,11 @@ describe("Error handling", () => {
       challengeDice: 0,
       forceDice: 0,
     });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "invalid" - number not found',
+    );
   });
+
   test("handles malformed dice numbers", () => {
     expect(parseDiceNotation("ayg")).toEqual({
       boostDice: 0,
@@ -315,6 +329,192 @@ describe("Error handling", () => {
       challengeDice: 0,
       forceDice: 0,
     });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "ayg" - number not found',
+    );
+  });
+
+  test("handles invalid numbers like 'abc'", () => {
+    expect(parseDiceNotation("abc")).toEqual({
+      boostDice: 0,
+      abilityDice: 0,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "abc" - number not found',
+    );
+  });
+
+  test("handles decimal numbers like '2.5g'", () => {
+    expect(parseDiceNotation("2.5g")).toEqual({
+      boostDice: 0,
+      abilityDice: 0,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "2.5g" - dice count must be a whole number',
+    );
+  });
+
+  test("handles comma numbers like '2,5g'", () => {
+    expect(parseDiceNotation("2,5g")).toEqual({
+      boostDice: 0,
+      abilityDice: 0,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "2,5g" - dice count must be a whole number',
+    );
+  });
+
+  test("handles invalid dice colors like '2x' or '3zz'", () => {
+    expect(parseDiceNotation("2x 3zz")).toEqual({
+      boostDice: 0,
+      abilityDice: 0,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice color: "x" in "2x"',
+    );
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice color: "zz" in "3zz"',
+    );
+  });
+
+  test("handles numbers without dice color like '5'", () => {
+    expect(parseDiceNotation("5")).toEqual({
+      boostDice: 0,
+      abilityDice: 0,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "5" - no dice color specified',
+    );
+  });
+
+  test("handles mixed valid and invalid notation", () => {
+    expect(parseDiceNotation("2g invalid 1p 3.5b 1r 2x")).toEqual({
+      boostDice: 0,
+      abilityDice: 2,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 1,
+      challengeDice: 1,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "invalid" - number not found',
+    );
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "3.5b" - dice count must be a whole number',
+    );
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice color: "x" in "2x"',
+    );
+  });
+
+  test("handles whitespace-only input", () => {
+    expect(parseDiceNotation("   ")).toEqual({
+      boostDice: 0,
+      abilityDice: 0,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).not.toHaveBeenCalled();
+  });
+
+  test("handles multiple spaces between valid dice", () => {
+    expect(parseDiceNotation("1g    2p")).toEqual({
+      boostDice: 0,
+      abilityDice: 1,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 2,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).not.toHaveBeenCalled();
+  });
+
+  test("handles special characters in notation", () => {
+    expect(parseDiceNotation("@#$% 1g !@#")).toEqual({
+      boostDice: 0,
+      abilityDice: 1,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "@#$%" - number not found',
+    );
+    expect(consoleErrorSpy).toHaveBeenCalledWith(
+      'Warning: Invalid dice notation: "!@#" - number not found',
+    );
+  });
+
+  test("handles negative numbers", () => {
+    expect(parseDiceNotation("-2g")).toEqual({
+      boostDice: 0,
+      abilityDice: -2,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    // Negative numbers are technically parsed but may not make sense semantically
+    expect(consoleErrorSpy).not.toHaveBeenCalled();
+  });
+
+  test("handles zero dice counts", () => {
+    expect(parseDiceNotation("0g 0p")).toEqual({
+      boostDice: 0,
+      abilityDice: 0,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).not.toHaveBeenCalled();
+  });
+
+  test("handles very large numbers", () => {
+    expect(parseDiceNotation("999999g")).toEqual({
+      boostDice: 0,
+      abilityDice: 999999,
+      proficiencyDice: 0,
+      setBackDice: 0,
+      difficultyDice: 0,
+      challengeDice: 0,
+      forceDice: 0,
+    });
+    expect(consoleErrorSpy).not.toHaveBeenCalled();
   });
 });
 
@@ -323,6 +523,8 @@ describe("CLI main function", () => {
   const mockConsoleLog = jest
     .spyOn(console, "log")
     .mockImplementation(() => {});
+  // Mock console.error - create a fresh spy for this test suite
+  let mockConsoleError: jest.SpyInstance;
   // Mock process.exit
   const mockExit = jest.spyOn(process, "exit").mockImplementation((number) => {
     throw new Error("process.exit: " + number);
@@ -332,10 +534,14 @@ describe("CLI main function", () => {
 
   beforeEach(() => {
     process.argv = ["node", "script.js"]; // Reset to default
+    mockConsoleError = jest
+      .spyOn(console, "error")
+      .mockImplementation(() => {});
   });
 
   afterEach(() => {
     mockConsoleLog.mockClear();
+    mockConsoleError.mockRestore();
     mockExit.mockClear();
   });
 
@@ -357,4 +563,27 @@ describe("CLI main function", () => {
     expect(() => main()).not.toThrow();
     expect(mockConsoleLog).toHaveBeenCalled();
   });
+
+  test("shows error message for completely invalid notation", () => {
+    process.argv = ["node", "script.js", "invalid", "xyz"];
+    expect(() => main()).toThrow("process.exit: 1");
+    // Multiple console.error calls are made - check for the main error message
+    const errorCalls = mockConsoleError.mock.calls.map((call) => call[0]);
+    expect(errorCalls).toContain(
+      "\nError: No valid dice found in the notation.",
+    );
+  });
+
+  test("processes mixed valid and invalid notation", () => {
+    process.argv = ["node", "script.js", "1y", "invalid", "2g"];
+    // This should still work since it has some valid dice
+    expect(() => main()).not.toThrow();
+    // Check that warning was logged for invalid notation
+    const errorCalls = mockConsoleError.mock.calls.map((call) => call[0]);
+    expect(errorCalls).toContain(
+      'Warning: Invalid dice notation: "invalid" - number not found',
+    );
+    // Check that the result was still displayed
+    expect(mockConsoleLog).toHaveBeenCalled();
+  });
 });