From 096f08c866da13dca75ca9963d3fd1c702370a68 Mon Sep 17 00:00:00 2001 From: Dai Ha Date: Thu, 1 Oct 2026 18:25:08 +0200 Subject: [PATCH] fleetd #635 follow-up: fix the stale --restore not-found message (defect 6, criterion 14) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The --restore "no backup found" message still printed the old beside-the-config glob (${CONFIG}.bak.*) even though newest_backup had already moved to searching the managed .config-backups/ directory. The message was left behind when the search moved — the search itself was already correct (ticket comment 17664). Fix is reporting-only: the message now names the directory actually searched (via backup_dir_for), and separately says that a backup written the old way, directly beside the config, is not searched any more, with the one-line cp to recover one by hand. No search fallback was added — reading backups from outside the managed directory stays unsupported, as instructed. Acceptance criterion 14 proves both directions: the not-found message names the real directory (confirmed red on the pre-fix code, green after), and a restore with a real backup present in .config-backups/ still succeeds (confirmed this catches an "always not-found" regression that direction 1 alone would miss). All 14 criteria plus 3 extras pass in scripts/test-config-edit.sh. --- scripts/config-edit.sh | 17 +++++++++++++-- scripts/test-config-edit.sh | 42 +++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 2 deletions(-) diff --git a/scripts/config-edit.sh b/scripts/config-edit.sh index 604cf7a..52e4c0f 100755 --- a/scripts/config-edit.sh +++ b/scripts/config-edit.sh @@ -612,9 +612,22 @@ dry_run_diff() { restore_mode() { [ -f "$CONFIG" ] || die "no config at $CONFIG to restore onto" - local backup + local backup dir base backup="$(newest_backup "$CONFIG")" - [ -n "$backup" ] || die "no backup found matching ${CONFIG}.bak.* — nothing to restore" + if [ -z "$backup" ]; then + # fleetd #635 follow-up (ticket comment 17664) — this message must name the directory the + # code actually searches (backup_dir_for, same as newest_backup), not the old beside-the- + # config glob. A backup written the OLD way is real and NOT searched any more — say so and + # give the one-line recovery command — but do NOT make the search itself look there; that + # would be a behaviour change nobody asked for. The message is the only thing being fixed. + dir="$(backup_dir_for "$CONFIG")" + base="$(basename "$CONFIG")" + die "no backup found matching ${dir}/${base}.bak.* — nothing to restore. + A backup written the OLD way, directly beside the config (${CONFIG}.bak.*), is NOT + searched — that location was retired so a backup of a file that must never be committed + cannot sit next to a tracked directory. If one exists there, recover it by hand: + cp ${CONFIG}.bak.. $CONFIG" + fi [ -f "$backup" ] || die "backup candidate $backup vanished" say "restore" diff --git a/scripts/test-config-edit.sh b/scripts/test-config-edit.sh index 20bd1cd..138b0fa 100755 --- a/scripts/test-config-edit.sh +++ b/scripts/test-config-edit.sh @@ -342,6 +342,46 @@ test_file_mode_survives_edit_and_restore() { done } +# ----------------------------------------- acceptance criterion 14: restore message names the real directory +# fleetd #635 follow-up (ticket comment 17664, defect 6) — the --restore "no backup found" +# message used to print the OLD beside-the-config glob even though newest_backup had already +# moved to searching the managed directory. Proves BOTH directions: the not-found message names +# the directory actually searched (not merely that it says SOMETHING), and that a real backup +# sitting in that directory still lets --restore succeed — otherwise the fix could regress into +# a message that is always printed regardless of whether a backup exists. +test_restore_message_names_the_searched_directory() { + local dir rc=0 + + # Direction 1: no backup anywhere — the message must name .config-backups/, not the bare + # beside-the-config glob the OLD code printed. + dir="$(new_fixture)" + "$EDIT" --restore --config "$dir/fleetd.yaml" --log "$dir/fleetd.out" --wait-seconds 2 \ + > "$dir/stdout.log" 2>&1 || rc=$? + RUN_OUTPUT="$(cat "$dir/stdout.log")" + + assert_equals 1 "$rc" "--restore with no backup anywhere exit code" + assert_contains ".config-backups/fleetd.yaml.bak.*" "$RUN_OUTPUT" \ + "the not-found message must name the directory actually searched, not the old beside-the-config glob" + + # Direction 2: a real backup IS present in .config-backups/ — --restore must still succeed, so + # the message fix cannot have turned into one that prints regardless of whether a backup exists. + dir="$(new_fixture)" + cp "$dir/fleetd.yaml" "$dir/pre-edit.yaml" + start_run "$dir" 5 --set '.profiles.sonnet.weight=77' + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "setup edit exit code for criterion 14's second half" + + start_run "$dir" 5 --restore + sleep 1 + printf 'config reloaded\n' >> "$dir/fleetd.out" + collect_run "$dir" + assert_equals 0 "$RUN_RC" "--restore with a real backup present must still succeed" + cmp -s "$dir/fleetd.yaml" "$dir/pre-edit.yaml" \ + || fail "--restore with a real backup present must put the file back byte for byte" +} + # dry-run must never touch the live file and must still redact. test_dry_run_never_installs_and_redacts() { local dir before @@ -409,6 +449,8 @@ echo "== acceptance criterion 11: a backup is never committable ==" test_backup_is_never_committable echo "== acceptance criterion 12: the file mode survives an edit and a restore ==" test_file_mode_survives_edit_and_restore +echo "== acceptance criterion 14: the restore message names the directory actually searched ==" +test_restore_message_names_the_searched_directory echo "== extra: dry-run never installs, and redacts ==" test_dry_run_never_installs_and_redacts echo "== extra: --check is read-only and always exits 0 =="