diff --git a/.gitignore b/.gitignore index 285cf65..605e1fb 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,6 @@ node_modules/ todos.json +*.tmp .DS_Store # Personal Claude Code overrides (not shared) diff --git a/src/cli.js b/src/cli.js index 90ffe77..638c469 100755 --- a/src/cli.js +++ b/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); -let todos = load(FILE); +const USAGE = "Usage: todo [ls | add | done | rm ]"; +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 | done | rm ]`); - process.exit(1); +function fail(message) { + console.error(message); + process.exit(1); } -save(FILE, todos); +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)); diff --git a/src/todo.js b/src/todo.js index efb8343..267188e 100644 --- a/src/todo.js +++ b/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)); } diff --git a/test/cli.test.js b/test/cli.test.js new file mode 100644 index 0000000..9d9d5b8 --- /dev/null +++ b/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+/); +}); diff --git a/test/todo.test.js b/test/todo.test.js index 01687e7..af2cfd7 100644 --- a/test/todo.test.js +++ b/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/); + }); +}