Address review round 2: duplicate ids, read-only files, dangling links, safe ids

- load() rejects duplicate ids (left by the old add bug) and ids that aren't safe positive integers
- save() refuses files the user can't write, and follows dangling symlinks to their target
- CLI rejects ids beyond Number.MAX_SAFE_INTEGER
- Skip permission-based tests when running as root

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

test/cli.test.js

@@ -41,6 +41,7 @@ for (const [label, args, message] of [
   ["decimal id", ["1.0"], /Invalid id: 1\.0/],
   ["padded id", [" 1 "], /Invalid id:  1 /],
   ["empty id", [""], /Invalid id: \n/],
+  ["unsafe id", ["99999999999999999999"], /Invalid id: 99999999999999999999/],
   ["unknown id", ["99"], /No todo with id 99/],
 ]) {
   for (const cmd of ["done", "rm"]) {
@@ -64,7 +65,7 @@ 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", () => {
+test("a failed save gives a readable error instead of a stack trace", { skip: process.getuid?.() === 0 && "root ignores file permissions" }, () => {
   const file = tempFile();
   chmodSync(dirname(file), 0o500);
   try {

test/todo.test.js

@@ -6,6 +6,7 @@ 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");
+const isRoot = process.getuid?.() === 0;
 
 test("add appends an open todo", () => {
   const todos = add([], "buy milk");
@@ -65,6 +66,29 @@ test("save writes through a symlink instead of replacing it", () => {
   assert.deepEqual(load(real).map((t) => t.text), ["a"]);
 });
 
+test("load rejects duplicate ids left by the old add() bug", () => {
+  const file = tempFile();
+  writeFileSync(file, JSON.stringify([{ id: 2, text: "b", done: false }, { id: 2, text: "c", done: false }]));
+  assert.throws(() => load(file), /id 2 is used more than once/);
+});
+
+test("save through a dangling symlink creates the target and keeps the link", () => {
+  const dir = dirname(tempFile());
+  const link = join(dir, "link.json");
+  symlinkSync("real.json", link);
+  save(link, add([], "a"));
+  assert.equal(lstatSync(link).isSymbolicLink(), true);
+  assert.deepEqual(load(join(dir, "real.json")).map((t) => t.text), ["a"]);
+});
+
+test("save refuses a read-only file", { skip: isRoot && "root ignores file permissions" }, () => {
+  const file = tempFile();
+  save(file, []);
+  chmodSync(file, 0o444);
+  assert.throws(() => save(file, add([], "a")), /Could not save .*todos\.json/);
+  assert.deepEqual(load(file), []);
+});
+
 test("save keeps the file's permissions", () => {
   const file = tempFile();
   save(file, []);
@@ -73,7 +97,7 @@ test("save keeps the file's permissions", () => {
   assert.equal(statSync(file).mode & 0o777, 0o600);
 });
 
-test("save reports a readable error and cleans up when it can't write", () => {
+test("save reports a readable error and cleans up when it can't write", { skip: isRoot && "root ignores file permissions" }, () => {
   const file = tempFile();
   chmodSync(dirname(file), 0o500);
   try {
@@ -96,7 +120,15 @@ test("load rejects JSON that is not a list", () => {
   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}']) {
+for (const entry of [
+  "null",
+  "1",
+  '{"text":"a","done":false}',
+  '{"id":"1","text":"a","done":false}',
+  '{"id":0,"text":"a","done":false}',
+  '{"id":-1,"text":"a","done":false}',
+  `{"id":${Number.MAX_SAFE_INTEGER},"text":"a","done":false}`,
+]) {
   test(`load rejects an invalid entry: ${entry}`, () => {
     const file = tempFile();
     writeFileSync(file, `[${entry}]`);