Merge pull request 'Fix duplicate ids, validate CLI input, harden load/save' (#2) from fix/review-findings into main

Reviewed-on: #2

.claude/settings.json

@@ -16,7 +16,11 @@
       "Read(~/.aws/**)",
       "Read(~/.gnupg/**)",
       "Edit(.claude/settings.json)",
-      "Edit(.claude/settings.local.json)"
+      "Edit(.claude/settings.local.json)",
+      "Bash(git push origin main:*)",
+      "Bash(git push origin HEAD:main:*)",
+      "Bash(git push * main:*)",
+      "Bash(git push *:main:*)"
     ],
     "blockReadsOutsideWorkingDirectories": true,
     "ask": [

.githooks/pre-commit

@@ -0,0 +1,10 @@
+#!/bin/sh
+# Refuse commits (and, via pre-merge-commit, merge commits) while main is checked out.
+# Cherry-pick, revert and fast-forward merges don't run these hooks; pre-push still stops
+# their result from reaching the remote.
+name=${1:-pre-commit}
+if [ "$(git symbolic-ref --short -q HEAD)" = "main" ]; then
+  echo "$name: committing on main is not allowed; create a branch first." >&2
+  exit 1
+fi
+exit 0

.githooks/pre-merge-commit

@@ -0,0 +1,2 @@
+#!/bin/sh
+exec sh "$(dirname "$0")/pre-commit" pre-merge-commit

.githooks/pre-push

@@ -0,0 +1,11 @@
+#!/bin/sh
+# Reject any push that would create, update or delete main on the remote, however the
+# command is spelled (bare `git push`, HEAD:main, +main, refs/heads/main, --all, --mirror).
+# Only a human merges into main, through a PR in Gitea.
+while read -r local_ref local_sha remote_ref remote_sha; do
+  if [ "$remote_ref" = "refs/heads/main" ]; then
+    echo "pre-push: pushing to main is not allowed; push a branch and open a PR instead." >&2
+    exit 1
+  fi
+done
+exit 0

.gitignore

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

CLAUDE.md

@@ -3,7 +3,8 @@
 A tiny zero-dependency Node.js todo CLI, used as a sandbox for trying Claude Code features.
 
 ## Commands
-- `npm test` — run the test suite (built-in `node:test`, no install needed)
+- `npm install` — once per clone (no dependencies): installs the git hooks that block commits and pushes to `main`. If it fails in the sandbox, ask the human to run it.
+- `npm test` — run the test suite (built-in `node:test`)
 - `npm start -- add "something"` / `npm start -- ls` — run the CLI (data in `todos.json`, override with `TODO_FILE`)
 
 ## Conventions

README.md

@@ -3,6 +3,7 @@
 A tiny todo CLI that exists to try out [Claude Code](https://docs.claude.com/en/docs/claude-code) features.
 
 ```sh
+npm install   # once per clone, and after changing .githooks/: installs the git hooks
 npm test
 npm start -- add "try Claude Code"
 npm start -- done 1
@@ -18,6 +19,14 @@ npm start -- ls
 | `.claude/skills/changelog/` | A custom skill — run `/changelog` |
 | `.claude/agents/test-writer.md` | A custom subagent — "use the test-writer agent to…" |
 
+### Protecting `main`
+Changes reach `main` only through a PR that a human merges. Three layers, strongest first:
+1. **Gitea branch protection** on `main` (set in the repo settings on code.westerkam.org): the only layer that can't be bypassed from a clone.
+2. **Git hooks** from `.githooks/`, copied into `.git/hooks` by `npm install` (`scripts/install-hooks.sh`). `pre-push` rejects any push that would update `main`, however it's spelled. `pre-commit` / `pre-merge-commit` refuse ordinary and merge commits while `main` is checked out; cherry-pick, revert and fast-forwards skip them, but pre-push still stops the result leaving the machine. The hooks are copied, not linked with `core.hooksPath`, because Claude Code's sandbox can't write inside `.git/`: a sandboxed command can't rewrite the installed hooks, which matters because `git commit` / `git push` run outside the sandbox. Anyone can still skip them with `--no-verify`.
+3. **Claude Code deny rules** in `.claude/settings.json` for `git push … main`: a quick early stop, but they match command text, so they're easy to get around.
+
+Layers 2 and 3 live on your machine and can be bypassed by whoever runs the commands; only layer 1 can't.
+
 ## Things to try
 
 - *"There's a bug in how ids are assigned after removing a todo — find it and fix it with a test."*

package.json

@@ -6,7 +6,8 @@
   "bin": { "todo": "./src/cli.js" },
   "scripts": {
     "start": "node src/cli.js",
-    "test": "node --test"
+    "test": "node --test",
+    "prepare": "sh scripts/install-hooks.sh"
   },
   "engines": { "node": ">=20" },
   "license": "MIT"

scripts/install-hooks.sh

@@ -0,0 +1,20 @@
+#!/bin/sh
+# Copy the hooks in .githooks/ into git's own hooks directory. They are copied rather than
+# enabled with core.hooksPath on purpose: Claude Code's sandbox can't write inside .git/, so
+# a sandboxed command can't rewrite the installed hooks, even though git commit and git push
+# (and so their hooks) run outside the sandbox. Re-run after changing anything in .githooks/.
+set -e
+cd "$(dirname "$0")/.."
+# Use the repo's own hooks directory, not `git rev-parse --git-path hooks`: that follows a
+# (possibly global) core.hooksPath and would install these hooks for every repo.
+if ! common=$(git rev-parse --git-common-dir 2>/dev/null); then
+  echo "install-hooks: not a git checkout; skipping." >&2
+  exit 0
+fi
+hooks="$common/hooks"
+mkdir -p "$hooks"
+for hook in .githooks/*; do
+  cp "$hook" "$hooks/"
+  chmod +x "$hooks/$(basename "$hook")"
+done
+echo "install-hooks: installed $(ls .githooks | tr '\n' ' ')into $hooks"

src/cli.js

@@ -1,28 +1,55 @@
 #!/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);
+const USAGE = "Usage: todo [ls | add <text> | done <id> | rm <id>]";
-let todos = load(FILE);
+const [cmd = "ls", ...args] = process.argv.slice(2);
 
-switch (cmd) {
+function fail(message) {
-  case "add":
+  console.error(message);
-    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>]`);
   process.exit(1);
 }
 
+function parseId(todos, arg) {
+  const id = Number(arg);
+  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;
+}
+
+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}`);
+    try {
+      todos = add(todos, text);
+    } catch (err) {
+      fail(err.message);
+    }
+    break;
+  }
+  case "done":
+    todos = complete(todos, parseId(todos, args[0]));
+    break;
+  case "rm":
+    todos = remove(todos, parseId(todos, args[0]));
+    break;
+}
+
+if (cmd !== "ls") {
+  try {
     save(FILE, todos);
+  } catch (err) {
+    fail(err.message);
+  }
+}
 console.log(format(todos));

src/todo.js

@@ -1,19 +1,99 @@
-import { readFileSync, writeFileSync, existsSync } from "node:fs";
+import {
+  readFileSync, writeFileSync, renameSync, existsSync, lstatSync, readlinkSync, statSync, chmodSync, chownSync, accessSync, rmSync,
+  constants,
+} from "node:fs";
+import { dirname, resolve } from "node:path";
+import { randomBytes } from "node:crypto";
+
+// 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";
 
 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`);
+  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`);
+  // Older versions could give two todos the same id. Keep the first and renumber the rest
+  // after the current highest id, so every todo can be targeted on its own again.
+  const seen = new Set();
+  let next = todos.reduce((max, t) => Math.max(max, t.id), 0);
+  return todos.map((t) => {
+    if (!seen.has(t.id)) {
+      seen.add(t.id);
+      return t;
+    }
+    if (!isId(++next)) throw new Error(`Could not read ${path}: no ids left to renumber duplicate id ${t.id}`);
+    return { ...t, id: next };
+  });
 }
 
+// Atomic save: write a uniquely named temp file next to the real file, then rename it into
+// place, so an interrupted write never truncates the list.
+// - Symlinks are followed by hand, so a dangling link gets its target created, not replaced.
+// - The temp file gets the real file's permissions from the start (so a private list is never
+//   readable by others mid-write) and its owner and group.
+// - A file the user can't write (chmod 444) is refused, as a plain write would be.
+// - Where a rename can't keep the file's identity (other hard links, an owner or group we
+//   can't assign, a directory we can't write), it falls back to a plain in-place write.
 export function save(path, todos) {
-  writeFileSync(path, JSON.stringify(todos, null, 2) + "\n");
+  const data = JSON.stringify(todos, null, 2) + "\n";
+  let tmp;
+  try {
+    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));
+    }
+    const stat = statSync(target, { throwIfNoEntry: false });
+    if (stat) accessSync(target, constants.W_OK);
+    const writeInPlace = () => {
+      if (tmp) rmSync(tmp, { force: true });
+      tmp = undefined;
+      writeFileSync(target, data);
+    };
+    if (stat?.nlink > 1) return writeInPlace();
+    const mode = stat ? stat.mode & 0o7777 : 0o666;
+    tmp = `${target}.${process.pid}.${randomBytes(4).toString("hex")}.tmp`;
+    try {
+      writeFileSync(tmp, data, { mode, flag: "wx" });
+    } catch (err) {
+      if (stat && (err.code === "EACCES" || err.code === "EPERM")) return writeInPlace();
+      throw err;
+    }
+    if (stat) {
+      try {
+        chownSync(tmp, stat.uid, stat.gid);
+      } catch {
+        return writeInPlace();
+      }
+      chmodSync(tmp, mode); // after chown, which can clear setgid; also undoes the umask
+    }
+    renameSync(tmp, target);
+  } catch (err) {
+    if (tmp) rmSync(tmp, { force: true });
+    throw new Error(`Could not save ${path}: ${err.message}`);
+  }
 }
 
 export function add(todos, text) {
-  const id = todos.length + 1;
+  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 }];
 }
 
+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,122 @@
+import { test } from "node:test";
+import assert from "node:assert/strict";
+import { spawnSync } from "node:child_process";
+import { mkdtempSync, writeFileSync, readFileSync, existsSync, chmodSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join, dirname } 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("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("rm on a file with duplicate ids removes only the first of them", () => {
+  const file = tempFile();
+  writeFileSync(file, JSON.stringify([{ id: 2, text: "b", done: false }, { id: 2, text: "c", done: false }]));
+  const res = run(file, "rm", "2");
+  assert.equal(res.status, 0, res.stderr);
+  assert.equal(res.stdout.trim(), "[ ] 3. c");
+});
+
+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/],
+  ["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/],
+  ["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(`${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 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 {
+    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");
+  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/hooks.test.js

@@ -0,0 +1,98 @@
+import { test, after } from "node:test";
+import assert from "node:assert/strict";
+import { spawnSync } from "node:child_process";
+import { mkdtempSync, rmSync, cpSync, statSync, writeFileSync, existsSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { fileURLToPath } from "node:url";
+
+const ROOT = fileURLToPath(new URL("../", import.meta.url));
+const HOOKS = join(ROOT, ".githooks");
+const SHA = "1".repeat(40);
+const ZERO = "0".repeat(40);
+
+// Run git and the hooks without inheriting GIT_DIR & co, e.g. when tests run from inside a hook.
+const env = Object.fromEntries(Object.entries(process.env).filter(([k]) => !k.startsWith("GIT_")));
+const run = (cmd, args, opts = {}) => spawnSync(cmd, args, { env, encoding: "utf8", ...opts });
+
+const dirs = [];
+after(() => dirs.forEach((d) => rmSync(d, { recursive: true, force: true })));
+
+function repoOn(branch) {
+  const dir = mkdtempSync(join(tmpdir(), "todo-hooks-"));
+  dirs.push(dir);
+  const init = run("git", ["init", "-q", "-b", branch, dir]);
+  assert.equal(init.status, 0, `git init failed: ${init.stderr}`);
+  return dir;
+}
+
+function prePush(...lines) {
+  return run("sh", [join(HOOKS, "pre-push"), "origin", "git@example:repo.git"], {
+    input: lines.map((l) => l + "\n").join(""),
+  });
+}
+
+for (const [label, line] of [
+  ["an update", `refs/heads/fix/x ${SHA} refs/heads/main ${SHA}`],
+  ["a new ref", `refs/heads/fix/x ${SHA} refs/heads/main ${ZERO}`],
+  ["a deletion", `(delete) ${ZERO} refs/heads/main ${SHA}`],
+]) {
+  test(`pre-push rejects ${label} of main`, () => {
+    const res = prePush(line);
+    assert.equal(res.status, 1);
+    assert.match(res.stderr, /pushing to main is not allowed/);
+  });
+}
+
+test("pre-push rejects --all style pushes that include main", () => {
+  const res = prePush(`refs/heads/fix/x ${SHA} refs/heads/fix/x ${SHA}`, `refs/heads/main ${SHA} refs/heads/main ${SHA}`);
+  assert.equal(res.status, 1);
+});
+
+test("pre-push allows other branches, including ones named like main", () => {
+  const res = prePush(`refs/heads/fix/x ${SHA} refs/heads/fix/x ${ZERO}`, `refs/heads/maintenance ${SHA} refs/heads/maintenance ${SHA}`);
+  assert.equal(res.status, 0);
+});
+
+for (const hook of ["pre-commit", "pre-merge-commit"]) {
+  test(`${hook} rejects main, names itself, and allows other branches`, () => {
+    const onMain = run("sh", [join(HOOKS, hook)], { cwd: repoOn("main") });
+    assert.equal(onMain.status, 1);
+    assert.match(onMain.stderr, new RegExp(`^${hook}: committing on main is not allowed`));
+    assert.equal(run("sh", [join(HOOKS, hook)], { cwd: repoOn("fix/x") }).status, 0);
+  });
+}
+
+test("install-hooks copies every hook into .git/hooks as an executable", () => {
+  const repo = repoOn("fix/x");
+  cpSync(join(ROOT, "scripts"), join(repo, "scripts"), { recursive: true });
+  cpSync(HOOKS, join(repo, ".githooks"), { recursive: true });
+  const res = run("sh", [join(repo, "scripts", "install-hooks.sh")]);
+  assert.equal(res.status, 0, res.stderr);
+  for (const hook of ["pre-push", "pre-commit", "pre-merge-commit"]) {
+    assert.ok(statSync(join(repo, ".git", "hooks", hook)).mode & 0o100, `${hook} is not executable`);
+  }
+});
+
+test("install-hooks ignores a global core.hooksPath", () => {
+  const repo = repoOn("fix/x");
+  const elsewhere = mkdtempSync(join(tmpdir(), "todo-hooks-"));
+  dirs.push(elsewhere);
+  const globalConfig = join(elsewhere, "gitconfig");
+  writeFileSync(globalConfig, `[core]\n\thooksPath = ${elsewhere}\n`);
+  cpSync(join(ROOT, "scripts"), join(repo, "scripts"), { recursive: true });
+  cpSync(HOOKS, join(repo, ".githooks"), { recursive: true });
+  const res = run("sh", [join(repo, "scripts", "install-hooks.sh")], { env: { ...env, GIT_CONFIG_GLOBAL: globalConfig } });
+  assert.equal(res.status, 0, res.stderr);
+  assert.ok(existsSync(join(repo, ".git", "hooks", "pre-push")));
+  assert.equal(existsSync(join(elsewhere, "pre-push")), false);
+});
+
+test("install-hooks skips cleanly outside a git checkout", () => {
+  const dir = mkdtempSync(join(tmpdir(), "todo-hooks-"));
+  dirs.push(dir);
+  cpSync(join(ROOT, "scripts"), join(dir, "scripts"), { recursive: true });
+  const res = run("sh", [join(dir, "scripts", "install-hooks.sh")], { env: { ...env, GIT_CEILING_DIRECTORIES: tmpdir() } });
+  assert.equal(res.status, 0);
+  assert.match(res.stderr, /not a git checkout; skipping/);
+});

test/todo.test.js

@@ -1,12 +1,29 @@
 import { test } from "node:test";
 import assert from "node:assert/strict";
-import { add, complete, remove, format } from "../src/todo.js";
+import { mkdtempSync, writeFileSync, readdirSync, symlinkSync, linkSync, lstatSync, statSync, chmodSync } from "node:fs";
+import { tmpdir } from "node:os";
+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");
   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 +38,130 @@ 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("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);
+});
+
+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.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("load renumbers 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: true },
+    { id: 3, text: "d", done: false },
+  ]));
+  assert.deepEqual(load(file).map((t) => [t.id, t.text, t.done]), [[2, "b", false], [4, "c", true], [3, "d", false]]);
+});
+
+test("save writes in place when the file has other hard links", () => {
+  const file = tempFile();
+  const other = join(dirname(file), "other.json");
+  save(file, []);
+  linkSync(file, other);
+  save(file, add([], "a"));
+  assert.deepEqual(load(other).map((t) => t.text), ["a"]);
+});
+
+test("save writes in place when only the file, not its directory, is writable", { skip: isRoot && "root ignores file permissions" }, () => {
+  const file = tempFile();
+  save(file, []);
+  chmodSync(dirname(file), 0o500);
+  try {
+    save(file, add([], "a"));
+    assert.deepEqual(load(file).map((t) => t.text), ["a"]);
+    assert.deepEqual(readdirSync(dirname(file)), ["todos.json"]);
+  } finally {
+    chmodSync(dirname(file), 0o700);
+  }
+});
+
+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, []);
+  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", { skip: isRoot && "root ignores file permissions" }, () => {
+  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", () => {
+  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/);
+});
+
+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}]`);
+    assert.throws(() => load(file), /entry 1 is not a valid todo/);
+  });
+}