fleetd #635 follow-up: refuse empty --set values, gitignore backups, preserve file mode

Five fixes against PR #636, all verified by the lead's own review and reproduced here:

1. --set .a.b= (a forgotten value) is now refused outright instead of silently nulling the
   field — a null numeric config value falls back to its default rather than erroring, which
   widens capacity silently instead of failing loudly. A deliberate clear gets its own spelling,
   --set .a.b=null, which writes a literal YAML null via yq, never through strenv(). (criteria
   9, 10)

2. Backups move from beside fleetd.yaml to a dedicated fleetd/.config-backups/ directory,
   gitignored at the repo root (so it also covers scripts/test-config-edit.sh's own throwaway
   fixtures) and in fleetd/.gitignore, plus a fleetd.yaml.bak.* glob backstop for any stray
   backup written the old way. A backup of a file that must never be committed inherits that
   requirement. (criterion 11)

3. The live config's file mode now survives both an edit and a restore. mv from a mktemp
   candidate used to carry mktemp's 0600 onto the live path forever, and cp onto an existing
   file keeps the destination's mode, so a restore did not undo it either. (criterion 12)

4. A global CAND + single EXIT/INT/TERM trap prevents an uninstalled .config-edit.XXXXXX
   candidate from leaking if the script is interrupted mid-run. No acceptance criterion is
   gated on this — a reproducible leak could not be made to happen on demand — but it is cheap
   and obviously right.

5. Acceptance criterion 7's redaction check gained a positive control: it now asserts the
   output actually CONTAINS the redaction marker and the changed key, not only that it lacks
   the secret. The prior two assertions were negative-only and passed just as happily when the
   diff was never printed at all — confirmed by reproducing the lead's own mutation (deleting
   the redacted diff print on the edit path) and watching it survive the old test and get
   caught by the new one. (criterion 13)

All 13 acceptance criteria plus 3 extras pass in scripts/test-config-edit.sh. Criteria 9, 10,
11, 12 and 13 were each proven non-vacuous: criteria 9/10 by mutating the test's own expected
value and watching it fail by name, then reverting; criteria 11/12/13 by reverting or mutating
the corresponding fix in config-edit.sh and watching the matching criterion fail by name, then
restoring the fix and re-confirming a clean pass.

scripts/config-edit.sh

@@ -46,6 +46,13 @@
 #   scripts/config-edit.sh --dry-run --set <yq-path>=<value>
 #   scripts/config-edit.sh --restore
 #
+# `--set .a.b=` (an empty value — a forgotten typo) is REFUSED, not accepted as "clear the
+# field": a null value falls back to its default rather than erroring, which is silent, not
+# safe. To clear a key on purpose, write a literal null: `--set .a.b=null`. Every other value
+# is always written as a YAML string (via yq's strenv(), never spliced into the expression), so
+# there is currently no --set spelling for the literal three-character STRING "null" itself — use
+# --from for that rare case.
+#
 # Overrides (so this is drivable with no daemon — see scripts/test-config-edit.sh):
 #   --config <path>        default: fleetd/fleetd.yaml
 #   --log <path>           default: fleetd/fleetd.out
@@ -77,6 +84,16 @@ DRY_RUN=0
 SETS=()
 FROM_FILE=""
 
+# fleetd #635 follow-up — a signal (or any early exit while a candidate is still uninstalled) must
+# not leave a `.config-edit.XXXXXX` file sitting beside the live config forever. CAND is global
+# (never a function-local) on purpose: this ONE trap, set once, covers every path that ever
+# creates a candidate — run_edit and dry_run_diff both assign it, and clear it back to "" once the
+# file is consumed (installed, or explicitly removed), so a later, unrelated exit never retries a
+# path that already served its purpose.
+CAND=""
+cleanup_candidate() { [ -n "$CAND" ] && rm -f "$CAND" 2>/dev/null; return 0; }
+trap cleanup_candidate EXIT INT TERM
+
 say()  { printf '\n\033[1m== %s\033[0m\n' "$*"; }
 ok()   { printf '   ok    %s\n' "$*"; }
 warn() { printf '   WARN  %s\n' "$*"; }
@@ -114,7 +131,7 @@ while [ $# -gt 0 ]; do
     --wait-seconds)
       [ $# -ge 2 ] || die "--wait-seconds requires a number of seconds"
       WAIT_SECONDS_OVERRIDE="$2"; shift 2 ;;
-    -h|--help) sed -n '3,63p' "$SELF"; exit 0 ;;
+    -h|--help) sed -n '3,70p' "$SELF"; exit 0 ;;
     *) echo "unknown option: $1 (try --help)" >&2; exit 2 ;;
   esac
 done
@@ -272,18 +289,62 @@ default_wait_seconds() {
 #
 # Timestamped, never pruned — "keep backups" per the ticket. A pid suffix avoids a same-second
 # collision between two invocations.
+#
+# fleetd #635 follow-up — lands under a DEDICATED, gitignored directory beside the config
+# (<dir>/.config-backups/), never beside the config file itself. The whole reason fleetd.yaml is
+# gitignored is that it must never be committed, and a backup of it inherits that requirement — a
+# bare `fleetd.yaml.bak.*` next to a tracked directory is one `git add -A`/`git add .` away from
+# committing the live config. A directory beats a glob on its own: the glob only protects today's
+# naming, a location keeps working even if the naming changes later. (See .gitignore for the glob
+# kept anyway, as a backstop for a stray backup written the old way.)
+BACKUP_DIRNAME=".config-backups"
+
+backup_dir_for() {
+  local src="$1"
+  printf '%s/%s' "$(dirname "$src")" "$BACKUP_DIRNAME"
+}
+
 backup_config() {
-  local src="$1" ts backup
+  local src="$1" ts backup dir base
+  dir="$(backup_dir_for "$src")"
+  mkdir -p "$dir" \
+    || die "could not create the backup directory $dir — refusing to edit without a backup. The live config at $src was NOT touched."
   ts="$(date -u +%Y%m%dT%H%M%S)Z"
-  backup="${src}.bak.${ts}.$$"
+  base="$(basename "$src")"
+  backup="${dir}/${base}.bak.${ts}.$$"
   cp "$src" "$backup" \
     || die "could not create a backup at $backup — refusing to edit without one. The live config at $src was NOT touched."
   printf '%s' "$backup"
 }
 
 newest_backup() {
-  local cfg="$1"
-  ls -t "${cfg}".bak.* 2>/dev/null | head -1 || true
+  local cfg="$1" dir base
+  dir="$(backup_dir_for "$cfg")"
+  base="$(basename "$cfg")"
+  ls -t "${dir}/${base}".bak.* 2>/dev/null | head -1 || true
+}
+
+# ------------------------------------------------------------------------------------ the file mode
+#
+# fleetd #635 follow-up — `mv` from a mktemp candidate carries mktemp's 0600 onto the live path
+# forever (measured: 644 -> 600 after one --set), and a restore does not undo it either, because
+# `cp` onto an EXISTING file keeps the DESTINATION's mode, not the source's. Capture the live
+# file's mode before anything touches it, and reapply it to whatever lands on that path
+# afterwards — the candidate before install, and the config again after a restore — so an edit
+# changes the file's CONTENT only, never its permissions. BSD `stat -f '%Lp'` first (matches this
+# project's dev machine), GNU `stat -c '%a'` as the fallback. Prints nothing when the file does
+# not exist yet, so apply_mode then does nothing and a first-ever edit falls back to the normal
+# umask default rather than inventing a number.
+file_mode() {
+  local file="$1"
+  [ -f "$file" ] || return 0
+  stat -f '%Lp' "$file" 2>/dev/null || stat -c '%a' "$file" 2>/dev/null || true
+}
+
+apply_mode() {
+  local file="$1" mode="$2"
+  [ -n "$mode" ] || return 0
+  chmod "$mode" "$file" 2>/dev/null || true
 }
 
 # --------------------------------------------------------------------------- candidate builders
@@ -291,6 +352,23 @@ newest_backup() {
 # Never edit the live file in place. Each builder fills $1 (a temp file already sitting in the
 # SAME directory as the live config, so the later `mv` install is a rename, not a cross-device
 # copy — see run_edit).
+# fleetd #635 follow-up — a forgotten value (`--set .a.b=`, a plausible typo) must never be
+# accepted as "clear the field". `*=*` alone cannot tell "--set .a.b=" from "--set .a.b=7" apart
+# — both contain an `=` — so the guard has to look at the VALUE, not the shape of the argument.
+# An empty value refuses outright: nothing is installed, and the message names the likely cause
+# AND the two ways to actually mean it (clear on purpose, or an intentional empty string via
+# --from). Measured against the real daemon loader: a quoted empty string reads back as a null
+# field (`quoted empty -> OK int=null`), and a null numeric field FALLS BACK TO ITS DEFAULT rather
+# than erroring — so this is not a cosmetic nit, it is the one shape of edit that widens capacity
+# silently instead of failing loudly, which is exactly what this script exists to catch.
+#
+# A deliberate clear needs its own spelling, because `""` and YAML `null` are NOT the same value
+# to the loader (`""` is a valid empty String; `null` means absent, and an Integer field reads
+# either the same way — null — but a String field would keep `""` as a real value). `--set
+# .a.b=null` is that spelling: it writes a literal, unquoted `null` via yq, never the string
+# "null" through strenv(). One consequence worth knowing: there is currently no --set spelling
+# for the three-character STRING "null" itself (it collides with the clear spelling) — use
+# --from for that rare case.
 apply_set_pairs() {
   local cand="$1" kv path value
   shift
@@ -302,6 +380,17 @@ apply_set_pairs() {
     path="${kv%%=*}"
     path="${path#.}"
     value="${kv#*=}"
+    if [ -z "$value" ]; then
+      die "--set '$kv' has an EMPTY value — refusing. Nothing was installed. A forgotten value
+          would NULL the field, and a null value falls back to its default rather than erroring —
+          silent, not safe. Did you mean --set .${path}=null to clear it on purpose, or --from a
+          file if you need a genuinely empty string?"
+    fi
+    if [ "$value" = "null" ]; then
+      yq eval -i ".${path} = null" "$cand" \
+        || die "yq could not clear '$kv' — nothing was installed. The live config is unchanged."
+      continue
+    fi
     CONFIG_EDIT_SET_VALUE="$value" yq eval -i ".${path} = strenv(CONFIG_EDIT_SET_VALUE)" "$cand" \
       || die "yq could not apply --set '$kv' — nothing was installed. The live config is unchanged."
   done
@@ -340,10 +429,12 @@ restore_command_line() {
 # restore itself reloaded cleanly. Never claims a restore it did not observe — if the second wait
 # also times out, it says so plainly rather than reporting "restored" as though confirmed.
 restore_and_confirm() {
-  local backup="$1" mark2
+  local backup="$1" mark2 orig_mode
+  orig_mode="$(file_mode "$CONFIG")"
   mark2="$(log_mark "$LOG")"
   cp "$backup" "$CONFIG" \
     || die "could not restore $backup onto $CONFIG — the live config is left as the REFUSED edit. Fix this by hand immediately: cp \"$backup\" \"$CONFIG\""
+  apply_mode "$CONFIG" "$orig_mode"
   ok "restored from $backup"
   if wait_for_verdict "$LOG" "$mark2" "$WAIT_SECONDS"; then
     case "$VERDICT_KIND" in
@@ -448,8 +539,9 @@ run_edit() {
   local builder="$1"
   [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to edit"
 
-  local mark
+  local mark orig_mode
   mark="$(log_mark "$LOG")"
+  orig_mode="$(file_mode "$CONFIG")"
 
   say "probe"
   local port
@@ -467,25 +559,29 @@ run_edit() {
 
   say "candidate"
   local cand
-  cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \
+  CAND="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \
     || die "could not create a candidate temp file next to $CONFIG"
+  cand="$CAND"
   if ! "$builder" "$cand"; then
-    rm -f "$cand"
+    rm -f "$cand"; CAND=""
     die "could not build the candidate — nothing was installed. The live config at $CONFIG is unchanged."
   fi
 
   if ! parse_check "$cand"; then
-    rm -f "$cand"
+    rm -f "$cand"; CAND=""
     die "candidate does not parse as valid YAML — nothing was installed. The live config at $CONFIG is unchanged."
   fi
   ok "candidate parses"
 
+  apply_mode "$cand" "$orig_mode"
+
   say "change (redacted)"
   diff -u "$backup" "$cand" | redact || true
 
   say "install"
   install_candidate "$cand" "$CONFIG" \
     || die "could not install the candidate onto $CONFIG — the live config was NOT changed. The validated candidate is sitting at $cand; investigate before retrying."
+  CAND=""
   ok "installed: $CONFIG"
 
   local rc=0
@@ -497,19 +593,20 @@ dry_run_diff() {
   local builder="$1"
   [ -f "$CONFIG" ] || die "no config at $CONFIG — nothing to diff against"
   local cand
-  cand="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \
+  CAND="$(mktemp "$(dirname "$CONFIG")/.config-edit.XXXXXX")" \
     || die "could not create a candidate temp file next to $CONFIG"
+  cand="$CAND"
   if ! "$builder" "$cand"; then
-    rm -f "$cand"
+    rm -f "$cand"; CAND=""
     die "could not build the candidate — this was a --dry-run, nothing would have been installed either"
   fi
   if ! parse_check "$cand"; then
-    rm -f "$cand"
+    rm -f "$cand"; CAND=""
     die "candidate does not parse as valid YAML — this was a --dry-run, nothing would have been installed either"
   fi
   say "dry run — diff (redacted), nothing installed"
   diff -u "$CONFIG" "$cand" | redact || true
-  rm -f "$cand"
+  rm -f "$cand"; CAND=""
   return 0
 }
 
@@ -522,9 +619,11 @@ restore_mode() {
 
   say "restore"
   ok "restoring $backup onto $CONFIG"
-  local mark
+  local mark orig_mode
   mark="$(log_mark "$LOG")"
+  orig_mode="$(file_mode "$CONFIG")"
   cp "$backup" "$CONFIG" || die "could not copy $backup onto $CONFIG"
+  apply_mode "$CONFIG" "$orig_mode"
   ok "installed: $CONFIG"
 
   local rc=0