Address review round 3: id limit, private temp files, canonical ids

- add() refuses to create an id that load() would reject; the CLI reports it
- save() creates the temp file with the real file's mode (and O_EXCL), so a
  private list is never briefly world-readable; one stat call; symlink
  resolution inlined; comment states the rename trade-offs
- CLI rejects ids with leading zeros and id 0
- Tests for trimming, the bare 'todo' default, and the id limit

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

src/cli.js

@@ -12,7 +12,7 @@ function fail(message) {
 
 function parseId(todos, arg) {
   const id = Number(arg);
-  if (!/^\d+$/.test(arg ?? "") || !Number.isSafeInteger(id)) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`);
+  if (!/^[1-9]\d*$/.test(arg ?? "") || !Number.isSafeInteger(id)) fail(`Invalid id: ${arg ?? "(missing)"}\n${USAGE}`);
   if (!has(todos, id)) fail(`No todo with id ${id}`);
   return id;
 }
@@ -30,7 +30,11 @@ switch (cmd) {
   case "add": {
     const text = args.join(" ").trim();
     if (!text) fail(`Missing todo text\n${USAGE}`);
-    todos = add(todos, text);
+    try {
+      todos = add(todos, text);
+    } catch (err) {
+      fail(err.message);
+    }
     break;
   }
   case "done":

src/todo.js

@@ -4,22 +4,12 @@ import {
 import { dirname, resolve } from "node:path";
 import { randomBytes } from "node:crypto";
 
-// Ids are positive and stay below MAX_SAFE_INTEGER so add()'s max + 1 is always exact.
+// Ids are positive and below MAX_SAFE_INTEGER, so max + 1 in add() is always exact.
 const isId = (id) => Number.isSafeInteger(id) && id > 0 && id < Number.MAX_SAFE_INTEGER;
 
 const isTodo = (t) =>
   t !== null && typeof t === "object" && isId(t.id) && typeof t.text === "string" && typeof t.done === "boolean";
 
-// Follow symlinks by hand so a dangling link resolves to the file it points at.
-function resolveLinks(path) {
-  let target = path;
-  for (let hops = 0; lstatSync(target, { throwIfNoEntry: false })?.isSymbolicLink(); hops++) {
-    if (hops === 40) throw new Error("too many levels of symbolic links");
-    target = resolve(dirname(target), readlinkSync(target));
-  }
-  return target;
-}
-
 export function load(path) {
   if (!existsSync(path)) return [];
   let todos;
@@ -37,18 +27,27 @@ export function load(path) {
   return todos;
 }
 
-// Write a uniquely named temp file next to the real file (following symlinks), copy over
+// Atomic save: write a uniquely named temp file next to the real file, then rename it into
-// its permissions, then rename it into place, so an interrupted write never truncates it.
+// place, so an interrupted write never truncates the list.
-// A file the user can't write (e.g. chmod 444) is refused, as a plain write would be.
+// - Symlinks are followed by hand, so a dangling link gets its target created, not replaced.
+// - The temp file is created with the real file's permissions, so a private list is never
+//   readable by others, even mid-write. A file the user can't write (chmod 444) is refused.
+// - Trade-offs of renaming: the directory must be writable, hard links to the old file are
+//   not updated, and the new file is owned by whoever ran the save.
 export function save(path, todos) {
   let tmp;
   try {
-    const target = resolveLinks(path);
+    let target = path;
-    const exists = existsSync(target);
+    for (let hops = 0; lstatSync(target, { throwIfNoEntry: false })?.isSymbolicLink(); hops++) {
-    if (exists) accessSync(target, constants.W_OK);
+      if (hops === 40) throw new Error("too many levels of symbolic links");
+      target = resolve(dirname(target), readlinkSync(target));
+    }
+    const stat = statSync(target, { throwIfNoEntry: false });
+    if (stat) accessSync(target, constants.W_OK);
+    const mode = stat ? stat.mode & 0o777 : 0o666;
     tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`;
-    writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n");
+    writeFileSync(tmp, JSON.stringify(todos, null, 2) + "\n", { mode, flag: "wx" });
-    if (exists) chmodSync(tmp, statSync(target).mode);
+    if (stat) chmodSync(tmp, mode); // undo the umask, so the mode matches exactly
     renameSync(tmp, target);
   } catch (err) {
     if (tmp) rmSync(tmp, { force: true });
@@ -58,6 +57,7 @@ export function save(path, todos) {
 
 export function add(todos, text) {
   const id = todos.reduce((max, t) => Math.max(max, t.id), 0) + 1;
+  if (!isId(id)) throw new Error("No ids left: the highest id is already the largest allowed");
   return [...todos, { id, text, done: false }];
 }
 

test/cli.test.js

@@ -25,6 +25,30 @@ test("ls with no file prints the empty message and writes nothing", () => {
   assert.equal(existsSync(file), false);
 });
 
+test("no command lists the todos", () => {
+  const file = tempFile();
+  run(file, "add", "a");
+  const res = run(file);
+  assert.equal(res.status, 0);
+  assert.equal(res.stdout.trim(), "[ ] 1. a");
+});
+
+test("add trims surrounding whitespace from the text", () => {
+  const file = tempFile();
+  run(file, "add", "  buy", "milk  ");
+  assert.deepEqual(JSON.parse(readFileSync(file, "utf8")), [{ id: 1, text: "buy milk", done: false }]);
+});
+
+test("add fails readably when no ids are left, without saving", () => {
+  const file = tempFile();
+  const before = JSON.stringify([{ id: Number.MAX_SAFE_INTEGER - 1, text: "a", done: false }]);
+  writeFileSync(file, before);
+  const res = run(file, "add", "b");
+  assert.equal(res.status, 1);
+  assert.match(res.stderr, /^No ids left/);
+  assert.equal(readFileSync(file, "utf8"), before);
+});
+
 test("add with no text fails without saving", () => {
   const file = tempFile();
   const res = run(file, "add", "  ");
@@ -42,6 +66,8 @@ for (const [label, args, message] of [
   ["padded id", [" 1 "], /Invalid id:  1 /],
   ["empty id", [""], /Invalid id: \n/],
   ["unsafe id", ["99999999999999999999"], /Invalid id: 99999999999999999999/],
+  ["leading-zero id", ["01"], /Invalid id: 01/],
+  ["zero id", ["0"], /Invalid id: 0/],
   ["unknown id", ["99"], /No todo with id 99/],
 ]) {
   for (const cmd of ["done", "rm"]) {

test/todo.test.js

@@ -43,6 +43,10 @@ test("load returns an empty list for a missing file", () => {
   assert.deepEqual(load(tempFile()), []);
 });
 
+test("add refuses an id that load() would reject", () => {
+  assert.throws(() => add([{ id: Number.MAX_SAFE_INTEGER - 1, text: "a", done: false }], "b"), /No ids left/);
+});
+
 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);