Address review: safer save, stricter load and id parsing

- save() follows symlinks, keeps permissions, uses a unique temp file and cleans it up on failure
- load() rejects entries that aren't {id, text, done}
- add() finds the max id with reduce (no spread limit)
- CLI accepts only plain digits as ids and reports save errors readably
- Ignore *.tmp

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

.gitignore

@@ -1,5 +1,6 @@
 node_modules/
 todos.json
+*.tmp
 .DS_Store
 
 # Personal Claude Code overrides (not shared)

src/cli.js

@@ -11,8 +11,8 @@ function fail(message) {
 }
 
 function parseId(todos, arg) {
+  if (!/^\d+$/.test(arg ?? "")) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`);
   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;
 }
@@ -41,5 +41,11 @@ switch (cmd) {
     break;
 }
 
-if (cmd !== "ls") save(FILE, todos);
+if (cmd !== "ls") {
+  try {
+    save(FILE, todos);
+  } catch (err) {
+    fail(err.message);
+  }
+}
 console.log(format(todos));

src/todo.js

@@ -1,4 +1,8 @@
-import { readFileSync, writeFileSync, renameSync, existsSync } from "node:fs";
+import { readFileSync, writeFileSync, renameSync, existsSync, realpathSync, statSync, chmodSync, rmSync } from "node:fs";
+import { randomBytes } from "node:crypto";
+
+const isTodo = (t) =>
+  t !== null && typeof t === "object" && Number.isInteger(t.id) && typeof t.text === "string" && typeof t.done === "boolean";
 
 export function load(path) {
   if (!existsSync(path)) return [];
@@ -9,18 +13,29 @@ export function load(path) {
     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`);
+  const bad = todos.findIndex((t) => !isTodo(t));
+  if (bad !== -1) throw new Error(`Could not read ${path}: entry ${bad + 1} is not a valid todo`);
   return todos;
 }
 
-// Write to a temp file and rename, so an interrupted write never leaves `path` truncated.
+// Write a uniquely named temp file next to the real file (following symlinks), copy over
+// its permissions, then rename it into place, so an interrupted write never truncates it.
 export function save(path, todos) {
-  const tmp = `${path}.tmp`;
-  writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n");
-  renameSync(tmp, path);
+  const exists = existsSync(path);
+  const target = exists ? realpathSync(path) : path;
+  const tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`;
+  try {
+    writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n");
+    if (exists) chmodSync(tmp, statSync(target).mode);
+    renameSync(tmp, target);
+  } catch (err) {
+    rmSync(tmp, { force: true });
+    throw new Error(`Could not save ${path}: ${err.message}`);
+  }
 }
 
 export function add(todos, text) {
-  const id = Math.max(0, ...todos.map((t) => t.id)) + 1;
+  const id = todos.reduce((max, t) => Math.max(max, t.id), 0) + 1;
   return [...todos, { id, text, done: false }];
 }
 

test/cli.test.js

@@ -1,9 +1,9 @@
 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 { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync } from "node:fs";
 import { tmpdir } from "node:os";
-import { join } from "node:path";
+import { join, dirname } from "node:path";
 import { fileURLToPath } from "node:url";
 
 const CLI = fileURLToPath(new URL("../src/cli.js", import.meta.url));
@@ -36,6 +36,11 @@ test("add with no text fails without saving", () => {
 for (const [label, args, message] of [
   ["missing id", [], /Invalid id: \(missing\)/],
   ["non-numeric id", ["abc"], /Invalid id: abc/],
+  ["hex id", ["0x1"], /Invalid id: 0x1/],
+  ["exponent id", ["1e0"], /Invalid id: 1e0/],
+  ["decimal id", ["1.0"], /Invalid id: 1\.0/],
+  ["padded id", [" 1 "], /Invalid id:  1 /],
+  ["empty id", [""], /Invalid id: \n/],
   ["unknown id", ["99"], /No todo with id 99/],
 ]) {
   for (const cmd of ["done", "rm"]) {
@@ -59,6 +64,19 @@ test("unknown command prints usage even when the file is corrupt", () => {
   assert.match(res.stderr, /Unknown command: --help\nUsage:/);
 });
 
+test("a failed save gives a readable error instead of a stack trace", () => {
+  const file = tempFile();
+  chmodSync(dirname(file), 0o500);
+  try {
+    const res = run(file, "add", "a");
+    assert.equal(res.status, 1);
+    assert.match(res.stderr, /^Could not save /);
+    assert.doesNotMatch(res.stderr, /at .*\.js:\d+/);
+  } finally {
+    chmodSync(dirname(file), 0o700);
+  }
+});
+
 test("a corrupt file gives a readable error instead of a stack trace", () => {
   const file = tempFile();
   writeFileSync(file, "{ not json");

test/todo.test.js

@@ -1,8 +1,8 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
-import { mkdtempSync, writeFileSync, existsSync } from "node:fs";
+import { mkdtempSync, writeFileSync, readdirSync, symlinkSync, lstatSync, statSync, chmodSync } from "node:fs";
 import { tmpdir } from "node:os";
-import { join } from "node:path";
+import { join, dirname } from "node:path";
 import { load, save, add, has, complete, remove, format } from "../src/todo.js";
 
 const tempFile = () => join(mkdtempSync(join(tmpdir(), "todo-")), "todos.json");
@@ -42,12 +42,46 @@ test("load returns an empty list for a missing file", () => {
   assert.deepEqual(load(tempFile()), []);
 });
 
+test("add handles a very large list", () => {
+  const todos = Array.from({ length: 200_000 }, (_, i) => ({ id: i + 1, text: "x", done: false }));
+  assert.equal(add(todos, "y").at(-1).id, 200_001);
+});
+
 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);
+  assert.deepEqual(readdirSync(dirname(file)), ["todos.json"]);
+});
+
+test("save writes through a symlink instead of replacing it", () => {
+  const real = tempFile();
+  const link = join(dirname(tempFile()), "link.json");
+  save(real, []);
+  symlinkSync(real, link);
+  save(link, add([], "a"));
+  assert.equal(lstatSync(link).isSymbolicLink(), true);
+  assert.deepEqual(load(real).map((t) => t.text), ["a"]);
+});
+
+test("save keeps the file's permissions", () => {
+  const file = tempFile();
+  save(file, []);
+  chmodSync(file, 0o600);
+  save(file, add([], "a"));
+  assert.equal(statSync(file).mode & 0o777, 0o600);
+});
+
+test("save reports a readable error and cleans up when it can't write", () => {
+  const file = tempFile();
+  chmodSync(dirname(file), 0o500);
+  try {
+    assert.throws(() => save(file, []), /Could not save .*todos\.json/);
+    assert.deepEqual(readdirSync(dirname(file)), []);
+  } finally {
+    chmodSync(dirname(file), 0o700);
+  }
 });
 
 test("load rejects invalid JSON with a readable error", () => {
@@ -61,3 +95,11 @@ test("load rejects JSON that is not a list", () => {
   writeFileSync(file, "{}");
   assert.throws(() => load(file), /expected a list of todos/);
 });
+
+for (const entry of ["null", "1", '{"text":"a","done":false}', '{"id":"1","text":"a","done":false}']) {
+  test(`load rejects an invalid entry: ${entry}`, () => {
+    const file = tempFile();
+    writeFileSync(file, `[${entry}]`);
+    assert.throws(() => load(file), /entry 1 is not a valid todo/);
+  });
+}