Fix duplicate ids, validate CLI input, harden load/save

- add() uses max id + 1 so ids are never reused after rm
- done/rm reject missing, non-numeric and unknown ids; add rejects empty text
- load() gives a readable error on bad JSON or non-list data
- save() writes a temp file and renames it
- CLI checks the command before loading and no longer relies on process.exit for ls
- Add regression tests for all of the above

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/cli.js

@@ -1,28 +1,45 @@
 #!/usr/bin/env node
-import { load, save, add, complete, remove, format } from "./todo.js";
+import { load, save, add, has, complete, remove, format } from "./todo.js";
 
 const FILE = process.env.TODO_FILE ?? "todos.json";
-const [cmd, ...args] = process.argv.slice(2);
-let todos = load(FILE);
+const USAGE = "Usage: todo [ls | add <text> | done <id> | rm <id>]";
+const [cmd = "ls", ...args] = process.argv.slice(2);
 
-switch (cmd) {
-  case "add":
-    todos = add(todos, args.join(" "));
-    break;
-  case "done":
-    todos = complete(todos, Number(args[0]));
-    break;
-  case "rm":
-    todos = remove(todos, Number(args[0]));
-    break;
-  case "ls":
-  case undefined:
-    console.log(format(todos));
-    process.exit(0);
-  default:
-    console.error(`Unknown command: ${cmd}\nUsage: todo [ls | add <text> | done <id> | rm <id>]`);
+function fail(message) {
+  console.error(message);
   process.exit(1);
 }
 
-save(FILE, todos);
+function parseId(todos, arg) {
+  const id = Number(arg);
+  if (arg === undefined || !Number.isInteger(id)) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`);
+  if (!has(todos, id)) fail(`No todo with id ${id}`);
+  return id;
+}
+
+if (!["ls", "add", "done", "rm"].includes(cmd)) fail(`Unknown command: ${cmd}\n${USAGE}`);
+
+let todos;
+try {
+  todos = load(FILE);
+} catch (err) {
+  fail(err.message);
+}
+
+switch (cmd) {
+  case "add": {
+    const text = args.join(" ").trim();
+    if (!text) fail(`Missing todo text\n${USAGE}`);
+    todos = add(todos, text);
+    break;
+  }
+  case "done":
+    todos = complete(todos, parseId(todos, args[0]));
+    break;
+  case "rm":
+    todos = remove(todos, parseId(todos, args[0]));
+    break;
+}
+
+if (cmd !== "ls") save(FILE, todos);
 console.log(format(todos));

src/todo.js

@@ -1,19 +1,33 @@
-import { readFileSync, writeFileSync, existsSync } from "node:fs";
+import { readFileSync, writeFileSync, renameSync, existsSync } from "node:fs";
 
 export function load(path) {
   if (!existsSync(path)) return [];
-  return JSON.parse(readFileSync(path, "utf8"));
+  let todos;
+  try {
+    todos = JSON.parse(readFileSync(path, "utf8"));
+  } catch (err) {
+    throw new Error(`Could not read ${path}: ${err.message}`);
+  }
+  if (!Array.isArray(todos)) throw new Error(`Could not read ${path}: expected a list of todos`);
+  return todos;
 }
 
+// Write to a temp file and rename, so an interrupted write never leaves `path` truncated.
 export function save(path, todos) {
-  writeFileSync(path, JSON.stringify(todos, null, 2) + "\n");
+  const tmp = `${path}.tmp`;
+  writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n");
+  renameSync(tmp, path);
 }
 
 export function add(todos, text) {
-  const id = todos.length + 1;
+  const id = Math.max(0, ...todos.map((t) => t.id)) + 1;
   return [...todos, { id, text, done: false }];
 }
 
+export function has(todos, id) {
+  return todos.some((t) => t.id === id);
+}
+
 export function complete(todos, id) {
   return todos.map((t) => (t.id === id ? { ...t, done: true } : t));
 }

test/cli.test.js

@@ -0,0 +1,69 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { spawnSync } from "node:child_process";
+import { mkdtempSync, writeFileSync, readFileSync, existsSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { fileURLToPath } from "node:url";
+
+const CLI = fileURLToPath(new URL("../src/cli.js", import.meta.url));
+
+function run(file, ...args) {
+  return spawnSync(process.execPath, [CLI, ...args], {
+    env: { ...process.env, TODO_FILE: file },
+    encoding: "utf8",
+  });
+}
+
+const tempFile = () => join(mkdtempSync(join(tmpdir(), "todo-")), "todos.json");
+
+test("ls with no file prints the empty message and writes nothing", () => {
+  const file = tempFile();
+  const res = run(file, "ls");
+  assert.equal(res.status, 0);
+  assert.equal(res.stdout.trim(), "Nothing to do.");
+  assert.equal(existsSync(file), false);
+});
+
+test("add with no text fails without saving", () => {
+  const file = tempFile();
+  const res = run(file, "add", "  ");
+  assert.equal(res.status, 1);
+  assert.match(res.stderr, /Missing todo text/);
+  assert.equal(existsSync(file), false);
+});
+
+for (const [label, args, message] of [
+  ["missing id", [], /Invalid id: \(missing\)/],
+  ["non-numeric id", ["abc"], /Invalid id: abc/],
+  ["unknown id", ["99"], /No todo with id 99/],
+]) {
+  for (const cmd of ["done", "rm"]) {
+    test(`${cmd} with ${label} fails and leaves the file unchanged`, () => {
+      const file = tempFile();
+      run(file, "add", "a");
+      const before = readFileSync(file, "utf8");
+      const res = run(file, cmd, ...args);
+      assert.equal(res.status, 1);
+      assert.match(res.stderr, message);
+      assert.equal(readFileSync(file, "utf8"), before);
+    });
+  }
+}
+
+test("unknown command prints usage even when the file is corrupt", () => {
+  const file = tempFile();
+  writeFileSync(file, "{ not json");
+  const res = run(file, "--help");
+  assert.equal(res.status, 1);
+  assert.match(res.stderr, /Unknown command: --help\nUsage:/);
+});
+
+test("a corrupt file gives a readable error instead of a stack trace", () => {
+  const file = tempFile();
+  writeFileSync(file, "{ not json");
+  const res = run(file, "ls");
+  assert.equal(res.status, 1);
+  assert.match(res.stderr, /^Could not read /);
+  assert.doesNotMatch(res.stderr, /at .*\.js:\d+/);
+});

test/todo.test.js

@@ -1,12 +1,28 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
-import { add, complete, remove, format } from "../src/todo.js";
+import { mkdtempSync, writeFileSync, existsSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { load, save, add, has, complete, remove, format } from "../src/todo.js";
+
+const tempFile = () => join(mkdtempSync(join(tmpdir(), "todo-")), "todos.json");
 
 test("add appends an open todo", () => {
   const todos = add([], "buy milk");
   assert.deepEqual(todos, [{ id: 1, text: "buy milk", done: false }]);
 });
 
+test("add after a remove does not reuse an existing id", () => {
+  const todos = add(remove(add(add([], "a"), "b"), 1), "c");
+  assert.deepEqual(todos.map((t) => t.id), [2, 3]);
+});
+
+test("has reports whether an id exists", () => {
+  const todos = add([], "a");
+  assert.equal(has(todos, 1), true);
+  assert.equal(has(todos, 2), false);
+});
+
 test("complete marks only the matching todo", () => {
   const todos = complete(add(add([], "a"), "b"), 2);
   assert.equal(todos[0].done, false);
@@ -21,3 +37,27 @@ test("remove drops the matching todo", () => {
 test("format renders an empty list", () => {
   assert.equal(format([]), "Nothing to do.");
 });
+
+test("load returns an empty list for a missing file", () => {
+  assert.deepEqual(load(tempFile()), []);
+});
+
+test("save then load round-trips and leaves no temp file", () => {
+  const file = tempFile();
+  const todos = add([], "a");
+  save(file, todos);
+  assert.deepEqual(load(file), todos);
+  assert.equal(existsSync(`${file}.tmp`), false);
+});
+
+test("load rejects invalid JSON with a readable error", () => {
+  const file = tempFile();
+  writeFileSync(file, "{ not json");
+  assert.throws(() => load(file), /Could not read .*todos\.json/);
+});
+
+test("load rejects JSON that is not a list", () => {
+  const file = tempFile();
+  writeFileSync(file, "{}");
+  assert.throws(() => load(file), /expected a list of todos/);
+});