From fb62fdfc8eff607273a50f0ce99cb36415384d2e Mon Sep 17 00:00:00 2001 From: Connor Johnstone Date: Tue, 22 Sep 2026 11:01:26 -0400 Subject: [PATCH] todo_rollover: atomic write, honest output, real argument handling MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hardening pass over the script. Every fix below was reproduced before being made; two findings that looked serious were tested and dropped. THE ONE THAT MATTERED. The write was not atomic and could destroy a day's todos. It did: cp -p "$src" "$dst" # yesterday's UNPRUNED note lands at the target cat "$tmp" > "$dst" # then gets overwritten with the real content Interrupted between those, today's note silently becomes yesterday's, unpruned. Interrupted during the redirect, it is truncated. Content is now built in a temp inside the notes directory and renamed over the target, so the target is either the old file or the new one and never a half-written mixture. ⚠ `cp -p` was NOT redundant, and a review pass that recommended removing it was wrong. A bare `cat > "$dst"` preserves the mode only when the target already exists; on the normal path the target is new and takes 0644 from the umask. Measured: source 600 -> new file via redirect 644, pre-existing file 600. The mode is now set explicitly with `chmod --reference` before the rename, which is correct in both cases. OUTPUT THAT LIED. The success line always said "(pruned)", including when the prune script was missing and the plain-copy fallback ran. That is the same defect as the error message that advertised a --force flag before one existed: a user-facing string asserting work the code did not do. The message now reports which path was taken, and a prune script that FAILS is reported instead of aborting on set -e with no explanation. ARGUMENT HANDLING. Anything that was not --force became a date, so `todo_rollover --dry-run` failed with "source note .../--dry-run.md not found" -- a missing-file error for what was really a bad flag. Unknown options are now rejected, `--` and `-h/--help` work, arity is checked, and dates are validated for shape AND realness (2026-13-45 is caught up front). ALSO FIXED - `[[ -x $PRUNE ]]` was the wrong test: PRUNE is run as `python3 "$PRUNE"` and needs no execute bit, so a working prune script was silently skipped. Now -f. - src == dst aborted on a raw `cp: ... are the same file`. Checked explicitly up front with a clear message. (It did fail safe -- a review pass claiming the source was destroyed was tested and is not reproducible.) - The heading regex rejected "# 2026-09-21 " with a trailing space and prepended a duplicate heading. Now tolerates surrounding whitespace. - `${tmp}.new` escaped the EXIT trap and leaked on interrupt. - Dead `mkdir -p "$NOTES"` after the source check had already proven the directory exists, and a `mode=` variable that was assigned and never used. Verified: unknown flag/bad date/missing source exit 1, existing target exits 2, --help exits 0, mode 600 survives the rollover, a non-executable prune script is used, a failing one leaves the target untouched, the trailing-space heading is retitled rather than duplicated, and no temp files are left behind. --- common/bin/todo_rollover | 112 ++++++++++++++++++++++++++++++--------- 1 file changed, 87 insertions(+), 25 deletions(-) diff --git a/common/bin/todo_rollover b/common/bin/todo_rollover index 5098e34..ddc5b2d 100755 --- a/common/bin/todo_rollover +++ b/common/bin/todo_rollover @@ -1,34 +1,84 @@ #!/usr/bin/env bash # On-demand rollover of a todo note. # By default copies yesterday's note to today, pruning finished work and retitling. -# Usage: todo_rollover [--force] [source_date] [target_date] +# +# Usage: todo_rollover [--force] [--] [source_date] [target_date] # source_date defaults to yesterday (YYYY-MM-DD) # target_date defaults to today # --force overwrites an existing target +# -h, --help show this message # Set TODO_NOTES to override notes directory. # Set TODO_PRUNE to override prune script. +# +# Exit codes: 1 bad input / missing source, 2 target exists (without --force). +# +# ⚠ The write is ATOMIC BY DESIGN: content is built in a temp file inside the +# notes directory, then renamed over the target. This file holds live todos, +# and the earlier version did `cp -p "$src" "$dst"` followed by +# `cat "$tmp" > "$dst"` -- which put YESTERDAY'S UNPRUNED note at the target +# path first, then overwrote it. Interrupted between the two, today's note +# silently became yesterday's; interrupted during the redirect, it was left +# truncated. Keep the rename. Do not "simplify" it back to a redirect. +# +# ⚠ `cp -p` was NOT redundant and removing it is a regression. A plain +# `cat > "$dst"` preserves the mode only when the target ALREADY EXISTS; +# on the normal path the target is new and gets 0644 from the umask instead +# of the source's mode. Measured 2026-09-22: source 600 -> new file via +# redirect 644, pre-existing file 600. Mode is now set explicitly on the temp +# before the rename, which covers both cases. set -euo pipefail NOTES=${TODO_NOTES:-$HOME/docs/notes/todo} PRUNE=${TODO_PRUNE:-$HOME/.local/lib/todo/prune.py} +usage() { sed -n '2,13p' "$0" | sed 's/^# \{0,1\}//'; } + force=0 args=() -for a in "$@"; do - if [[ $a == --force ]]; then - force=1 - else - args+=("$a") - fi +while (( $# )); do + case $1 in + --force) force=1 ;; + -h|--help) usage; exit 0 ;; + --) shift; args+=("$@"); break ;; + # ⚠ Reject unknown flags rather than treating them as a date. Previously + # `todo_rollover --dry-run` set source_date to "--dry-run" and failed with + # "source note .../--dry-run.md not found", which reads as a missing file + # rather than a bad flag. + -*) echo "todo_rollover: unknown option '$1' (try --help)" >&2; exit 1 ;; + *) args+=("$1") ;; + esac + shift done +if (( ${#args[@]} > 2 )); then + echo "todo_rollover: expected at most 2 dates, got ${#args[@]} (try --help)" >&2 + exit 1 +fi + src_date=${args[0]:-$(date -d yesterday +%Y-%m-%d)} target_date=${args[1]:-$(date +%Y-%m-%d)} +# Validate shape AND that the date is real, so 2026-13-45 fails here rather +# than as a confusing "source note not found". +for d in "$src_date" "$target_date"; do + if [[ ! $d =~ ^[0-9]{4}-[0-9]{2}-[0-9]{2}$ ]] || ! date -d "$d" >/dev/null 2>&1; then + echo "todo_rollover: '$d' is not a valid YYYY-MM-DD date" >&2 + exit 1 + fi +done + src="$NOTES/$src_date.md" dst="$NOTES/$target_date.md" +# ⚠ Explicit, BEFORE anything is written. `cp` happens to refuse a same-file +# copy, so the old code failed safe -- but by aborting mid-script on a raw +# `cp: ... are the same file` error. Say it plainly instead. +if [[ $src -ef $dst || $src == "$dst" ]]; then + echo "todo_rollover: source and target are the same file ($src)" >&2 + exit 1 +fi + if [[ ! -f $src ]]; then echo "todo_rollover: source note $src not found" >&2 exit 1 @@ -36,35 +86,47 @@ fi if [[ -f $dst && $force -eq 0 ]]; then echo "todo_rollover: target $dst already exists, skipping (use --force to overwrite)" >&2 - echo " Existing target was created from a previous rollover. If you really want to replace it, remove it first." >&2 exit 2 fi -mkdir -p "$NOTES" -tmp=$(mktemp) -trap 'rm -f "$tmp"' EXIT +# Temp lives in the notes directory so the final rename is same-filesystem, +# and therefore atomic. +tmp=$(mktemp "$NOTES/.todo_rollover.XXXXXX") +trap 'rm -f "$tmp" "$tmp.new"' EXIT -if [[ -x $PRUNE ]]; then - python3 "$PRUNE" "$src" > "$tmp" +# ⚠ Test -f, not -x. PRUNE is invoked as `python3 "$PRUNE"`, which does not +# need the execute bit; -x sent a perfectly good prune script down the +# no-prune fallback silently. +pruned=0 +if [[ -f $PRUNE ]]; then + if ! python3 "$PRUNE" "$src" > "$tmp"; then + echo "todo_rollover: prune script failed: python3 $PRUNE $src" >&2 + exit 1 + fi + pruned=1 else - # fallback: just copy cp "$src" "$tmp" fi -# Retitle first line to target date, matching rollover.py behavior -# Only replace line 1 if it looks like a date heading +# Retitle line 1 to the target date. Tolerates surrounding whitespace -- the +# stricter pattern treated "# 2026-09-21 " as not-a-heading and prepended a +# second one. first_line=$(head -n1 "$tmp") -if [[ $first_line =~ ^#\ [0-9]{4}-[0-9]{2}-[0-9]{2}$ ]]; then +if [[ $first_line =~ ^[[:space:]]*#[[:space:]]+[0-9]{4}-[0-9]{2}-[0-9]{2}[[:space:]]*$ ]]; then sed -i "1s/.*/# $target_date/" "$tmp" else - # prepend heading - printf "# %s\n\n" "$target_date" | cat - "$tmp" > "${tmp}.new" - mv "${tmp}.new" "$tmp" + printf "# %s\n\n" "$target_date" | cat - "$tmp" > "$tmp.new" + mv "$tmp.new" "$tmp" fi -# Preserve mode of source -mode=$(stat -c %a "$src") -cp -p "$src" "$dst" # to get mode -cat "$tmp" > "$dst" +chmod --reference="$src" "$tmp" +mv -f "$tmp" "$dst" -echo "todo_rollover: created $dst from $src_date.md (pruned)" +# ⚠ Only claim "(pruned)" when pruning actually ran. The previous version said +# it unconditionally, including on the no-prune fallback -- the same defect as +# an error message advertising a --force flag that was never implemented. +if (( pruned )); then + echo "todo_rollover: created $dst from $src_date.md (pruned)" +else + echo "todo_rollover: created $dst from $src_date.md (NOT pruned: $PRUNE not found)" +fi