Fix duplicate ids, validate CLI input, harden load/save #2

2026-10-21T00:19:26+02:00

christian commented

2026-10-21 00:19:26 +02:00

Fixes the 8 findings from the /code-review of src/:

add() uses max id + 1, so ids are never reused after rm
done/rm reject missing, non-numeric and unknown ids; add rejects empty text (exit 1, file untouched)
load() gives a readable error on bad JSON or non-list data
save() writes a temp file and renames it, so an interrupted write can't truncate todos.json
CLI checks the command before loading the file and no longer relies on process.exit for ls
Regression tests for all of the above (test/todo.test.js, new test/cli.test.js); npm test: 20/20 pass

🤖 Generated with Claude Code

Fixes the 8 findings from the `/code-review` of `src/`: - `add()` uses max id + 1, so ids are never reused after `rm` - `done`/`rm` reject missing, non-numeric and unknown ids; `add` rejects empty text (exit 1, file untouched) - `load()` gives a readable error on bad JSON or non-list data - `save()` writes a temp file and renames it, so an interrupted write can't truncate `todos.json` - CLI checks the command before loading the file and no longer relies on `process.exit` for `ls` - Regression tests for all of the above (`test/todo.test.js`, new `test/cli.test.js`); `npm test`: 20/20 pass 🤖 Generated with [Claude Code](https://claude.com/claude-code)

christian added 1 commit 2026-10-21 00:19:26 +02:00

Fix duplicate ids, validate CLI input, harden load/save ba17376132

- add() uses max id + 1 so ids are never reused after rm
- done/rm reject missing, non-numeric and unknown ids; add rejects empty text
- load() gives a readable error on bad JSON or non-list data
- save() writes a temp file and renames it
- CLI checks the command before loading and no longer relies on process.exit for ls
- Add regression tests for all of the above

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

christian commented

2026-10-21 00:20:22 +02:00

Needs fixes

Automated self-review by Claude (/code-review, medium). The same agent wrote this change, so this is not an independent review.

src/todo.js:18 — The rename-based save() replaces a symlinked todos.json with a regular file (writes stop reaching the link target) and resets file permissions.
src/todo.js:16 — The fixed temp name ${path}.tmp is shared by concurrent runs: one rename can fail with ENOENT, or one todo is silently lost. Use a unique temp name.
src/todo.js:12 — load() validates only that the file holds a list. [null] still crashes format(), and entries without a numeric id make add() produce NaN/null ids.
src/cli.js:45 — Errors from save() (read-only dir, disk full) aren't caught, so they print a raw stack trace and may leave the .tmp file behind.
src/todo.js:22 — Math.max(0, ...ids) throws RangeError on very large lists. Use reduce.
src/cli.js:15 — parseId accepts 0x2, 2e0, " 2 " and 1.0, and "" becomes id 0. Validate with /^\d+$/.
.gitignore:2 — todos.json.tmp isn't ignored, so a leftover temp file could be committed.
src/cli.js:16 — Cleanup: the arg === undefined check is redundant.

**Needs fixes** _Automated self-review by Claude (`/code-review`, medium). The same agent wrote this change, so this is not an independent review._ 1. `src/todo.js:18` — The rename-based `save()` replaces a symlinked `todos.json` with a regular file (writes stop reaching the link target) and resets file permissions. 2. `src/todo.js:16` — The fixed temp name `${path}.tmp` is shared by concurrent runs: one rename can fail with ENOENT, or one todo is silently lost. Use a unique temp name. 3. `src/todo.js:12` — `load()` validates only that the file holds a list. `[null]` still crashes `format()`, and entries without a numeric `id` make `add()` produce `NaN`/`null` ids. 4. `src/cli.js:45` — Errors from `save()` (read-only dir, disk full) aren't caught, so they print a raw stack trace and may leave the `.tmp` file behind. 5. `src/todo.js:22` — `Math.max(0, ...ids)` throws RangeError on very large lists. Use `reduce`. 6. `src/cli.js:15` — `parseId` accepts `0x2`, `2e0`, `" 2 "` and `1.0`, and `""` becomes id 0. Validate with `/^\d+$/`. 7. `.gitignore:2` — `todos.json.tmp` isn't ignored, so a leftover temp file could be committed. 8. `src/cli.js:16` — Cleanup: the `arg === undefined` check is redundant.

christian added 1 commit 2026-10-21 00:26:35 +02:00

Address review: safer save, stricter load and id parsing 8dc5194438

- 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>

christian commented

2026-10-21 00:27:34 +02:00

Needs fixes
Automated self-review by Claude; not an independent review.

Round 2 (after 8dc5194). The round-1 findings are fixed. New findings, most important first:

Correctness

src/todo.js:16 — load() doesn't reject duplicate ids. Files written by the old todos.length + 1 bug can already hold [2, 2], and rm 2 then deletes both todos.
src/todo.js:29 — Copying the mode onto the temp file and renaming it lets the CLI overwrite a chmod 444 (read-only) todo file that the old code refused to write.
src/todo.js:24 — A dangling symlink fails existsSync, so the rename replaces the link itself instead of creating its target.
src/cli.js:14 / src/todo.js:38 — Ids aren't checked to be safe integers: huge ids round silently, max + 1 can duplicate at MAX_SAFE_INTEGER, and negative ids load but can never be targeted.

Trade-offs / robustness (judgement calls)
5. src/todo.js:30 — Saving via temp file + rename needs write access to the directory, and drops hard links, owner/group, ACLs and xattrs.
6. src/todo.js:28 — There's no fsync before the rename, so after a power loss the file can still come back empty on some filesystems.
7. src/cli.js:9 — process.exit(1) right after console.error can cut off stderr on a slow pipe on macOS.

Tests
8. test/cli.test.js:69, test/todo.test.js:77 — The save-failure tests rely on chmod 0o500, which root ignores, so they fail when run as root in Docker CI.
9. test/todo.test.js:9 — Temp directories are never removed.

Fix round cap reached for this session: handing off to the human to decide what to fix.

**Needs fixes** _Automated self-review by Claude; not an independent review._ Round 2 (after 8dc5194). The round-1 findings are fixed. New findings, most important first: **Correctness** 1. `src/todo.js:16` — `load()` doesn't reject duplicate ids. Files written by the old `todos.length + 1` bug can already hold `[2, 2]`, and `rm 2` then deletes both todos. 2. `src/todo.js:29` — Copying the mode onto the temp file and renaming it lets the CLI overwrite a `chmod 444` (read-only) todo file that the old code refused to write. 3. `src/todo.js:24` — A dangling symlink fails `existsSync`, so the rename replaces the link itself instead of creating its target. 4. `src/cli.js:14` / `src/todo.js:38` — Ids aren't checked to be safe integers: huge ids round silently, `max + 1` can duplicate at `MAX_SAFE_INTEGER`, and negative ids load but can never be targeted. **Trade-offs / robustness** (judgement calls) 5. `src/todo.js:30` — Saving via temp file + rename needs write access to the directory, and drops hard links, owner/group, ACLs and xattrs. 6. `src/todo.js:28` — There's no fsync before the rename, so after a power loss the file can still come back empty on some filesystems. 7. `src/cli.js:9` — `process.exit(1)` right after `console.error` can cut off stderr on a slow pipe on macOS. **Tests** 8. `test/cli.test.js:69`, `test/todo.test.js:77` — The save-failure tests rely on `chmod 0o500`, which root ignores, so they fail when run as root in Docker CI. 9. `test/todo.test.js:9` — Temp directories are never removed. Fix round cap reached for this session: handing off to the human to decide what to fix.

christian added 1 commit 2026-10-21 00:34:21 +02:00

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

- 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>

christian commented

2026-10-21 00:35:29 +02:00

Needs fixes
Automated self-review by Claude; not an independent review.

Round 3 (after 089fc94). Round 2's correctness findings are fixed. New findings, most important first:

Correctness

src/todo.js:8 — add() can produce id MAX_SAFE_INTEGER, which isId then rejects, so the file becomes unloadable. Make add() refuse past the limit, or let isId allow <=.
src/todo.js:50 — The temp file is created with the umask mode (usually 0644) and chmod'ed only after writing, so a chmod 600 todo list is briefly readable by everyone. Pass { mode } to writeFileSync.
src/cli.js:15 — parseId accepts leading zeros (007 → 7), unlike every other non-canonical form.

Conventions (CLAUDE.md)
4. src/cli.js:31 — Trimming todo text and the bare todo → ls default have no tests ("Every behavior change gets a test").
5. src/todo.js:14 — resolveLinks does I/O outside load/save in the pure-functions module. Inline it into save().

Trade-offs (judgement calls, already raised in round 2)
6. src/todo.js:49, :52 — Temp file + rename needs a writable directory and drops hard links and ownership. The comment saying "as a plain write would be" overstates it.
7. src/cli.js:31 — No locking, so concurrent runs can lose an update. Atomic rename prevents truncation, not lost writes.
8. src/todo.js:47 — Several separate stat calls could be one statSync(…, { throwIfNoEntry: false }).

Not applicable to this PR: the .claude/settings.json deny-rule finding came from the uncommitted working tree and belongs to PR #3 (already raised there).

Fix round cap (2) reached: handing off to the human.

**Needs fixes** _Automated self-review by Claude; not an independent review._ Round 3 (after 089fc94). Round 2's correctness findings are fixed. New findings, most important first: **Correctness** 1. `src/todo.js:8` — `add()` can produce id `MAX_SAFE_INTEGER`, which `isId` then rejects, so the file becomes unloadable. Make `add()` refuse past the limit, or let `isId` allow `<=`. 2. `src/todo.js:50` — The temp file is created with the umask mode (usually 0644) and chmod'ed only after writing, so a `chmod 600` todo list is briefly readable by everyone. Pass `{ mode }` to `writeFileSync`. 3. `src/cli.js:15` — `parseId` accepts leading zeros (`007` → 7), unlike every other non-canonical form. **Conventions (CLAUDE.md)** 4. `src/cli.js:31` — Trimming todo text and the bare `todo` → `ls` default have no tests ("Every behavior change gets a test"). 5. `src/todo.js:14` — `resolveLinks` does I/O outside `load`/`save` in the pure-functions module. Inline it into `save()`. **Trade-offs** (judgement calls, already raised in round 2) 6. `src/todo.js:49`, `:52` — Temp file + rename needs a writable directory and drops hard links and ownership. The comment saying "as a plain write would be" overstates it. 7. `src/cli.js:31` — No locking, so concurrent runs can lose an update. Atomic rename prevents truncation, not lost writes. 8. `src/todo.js:47` — Several separate stat calls could be one `statSync(…, { throwIfNoEntry: false })`. Not applicable to this PR: the `.claude/settings.json` deny-rule finding came from the uncommitted working tree and belongs to PR #3 (already raised there). Fix round cap (2) reached: handing off to the human.

christian added 1 commit 2026-10-21 00:41:45 +02:00

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

- 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>

christian added 1 commit 2026-10-21 00:49:50 +02:00

Lock during updates, repair duplicate ids, keep file identity on save dab492da1e

- CLI holds a lock file from load to save, so concurrent commands can't
  lose an update; stale locks from crashed runs are taken over
- load() renumbers duplicate ids from the old add() bug instead of
  refusing the file
- save() keeps owner, group and setgid on the temp file, and falls back to
  an in-place write for hard-linked files, files whose owner/group can't
  be kept, and directories that aren't writable

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

christian commented

2026-10-21 00:53:45 +02:00

Needs fixes (stopping here: handing off to the human)
Automated self-review by Claude; not an independent review.

Round 4 (after dab492d). The round-3 findings are fixed. All 9 new findings are unverified candidates, and most are about the lock added in this round:

src/cli.js — Stale-lock takeover can race; the exit handler can delete another process's lock; symlinked paths get different locks; Ctrl-C leaves the lock behind; a read-only directory silently skips locking; the "busy" error path has no test.
src/todo.js — The in-place write fallback isn't atomic, so a concurrent ls could read a partial file; the max-id calculation is duplicated in load() and add().

Note: these findings keep shrinking in severity but not in number, mostly because each fix adds new code to review. A reasonable option is to drop the lock commit (dab492d's lock part) and accept that concurrent runs of a personal CLI can lose an update.

**Needs fixes** (stopping here: handing off to the human) _Automated self-review by Claude; not an independent review._ Round 4 (after dab492d). The round-3 findings are fixed. All 9 new findings are unverified candidates, and most are about the lock added in this round: - `src/cli.js` — Stale-lock takeover can race; the exit handler can delete another process's lock; symlinked paths get different locks; Ctrl-C leaves the lock behind; a read-only directory silently skips locking; the "busy" error path has no test. - `src/todo.js` — The in-place write fallback isn't atomic, so a concurrent `ls` could read a partial file; the max-id calculation is duplicated in `load()` and `add()`. **Note:** these findings keep shrinking in severity but not in number, mostly because each fix adds new code to review. A reasonable option is to drop the lock commit (dab492d's lock part) and accept that concurrent runs of a personal CLI can lose an update.

christian added 1 commit 2026-10-21 00:56:20 +02:00

Drop the lock file: too much machinery for a personal CLI 130ffe0d13

Concurrent runs of the CLI can again lose an update; saves stay atomic.

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

christian merged commit 14a5774d32 into main

2026-10-21 00:58:55 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: christian/claude-playground#2