fleetd #641: warn on --set reformat churn

scripts/config-edit.sh

@@ -2,6 +2,10 @@
 #
 # The one auditable way to edit the live fleetd.yaml.
 #
+# `--set` uses yq and rewrites the whole YAML document in yq's output style. Use `--from` for a
+# candidate whose comment alignment or other formatting carries meaning: it copies that file
+# verbatim while keeping this script's backup, parse check, atomic install, and verdict read-back.
+#
 # fleetd ticket #635 — why this exists at all: fleetd.yaml is gitignored and holds the live
 # fleet's settings. A bad raw edit reaches a daemon that is already serving, so a direct `Edit`
 # on it is refused by policy. This script is the allow-listed alternative, and it is not just
@@ -46,6 +50,11 @@
 #   scripts/config-edit.sh --dry-run --set <yq-path>=<value>
 #   scripts/config-edit.sh --restore
 #
+# `--set` rewrites the whole file in yq's output style, not only the requested keys. The script
+# warns before installation when the candidate changes more lines than its number of --set pairs.
+# Use `--from <candidate.yaml>` when comment alignment or other formatting is meaningful: --from
+# copies the candidate verbatim, with no yq round-trip.
+#
 # `--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
@@ -463,6 +472,50 @@ parse_check() {
   yq eval '.' "$1" >/dev/null 2>&1
 }
 
+# Count logical changed lines in a unified diff. A replacement counts once, while an added or
+# deleted line also counts once. One changed `--set` value normally produces one changed line.
+changed_line_count() {
+  local before="$1" after="$2" line count=0 old_count=0 new_count=0
+  while IFS= read -r line || [ -n "$line" ]; do
+    case "$line" in
+      ---\ *|+++\ *|@@\ *)
+        if [ "$old_count" -gt "$new_count" ]; then
+          count=$((count + old_count))
+        else
+          count=$((count + new_count))
+        fi
+        old_count=0
+        new_count=0
+        ;;
+      -*) old_count=$((old_count + 1)) ;;
+      +*) new_count=$((new_count + 1)) ;;
+      *)
+        if [ "$old_count" -gt "$new_count" ]; then
+          count=$((count + old_count))
+        else
+          count=$((count + new_count))
+        fi
+        old_count=0
+        new_count=0
+        ;;
+    esac
+  done < <(diff -u "$before" "$after" || true)
+  if [ "$old_count" -gt "$new_count" ]; then
+    count=$((count + old_count))
+  else
+    count=$((count + new_count))
+  fi
+  printf '%s' "$count"
+}
+
+warn_set_reformat() {
+  local before="$1" after="$2" changed
+  changed="$(changed_line_count "$before" "$after")"
+  if [ "$changed" -gt "${#SETS[@]}" ]; then
+    warn "--set changed $changed candidate lines for ${#SETS[@]} pair(s); yq reformatted the whole file. Use --from for meaningful comment alignment or formatting."
+  fi
+}
+
 install_candidate() {
   local cand="$1" live="$2"
   mv -f "$cand" "$live"
@@ -624,6 +677,10 @@ run_edit() {
   fi
   ok "candidate parses"
 
+  if [ "$MODE" = "set" ]; then
+    warn_set_reformat "$backup" "$cand"
+  fi
+
   apply_mode "$cand" "$orig_mode"
 
   say "change (redacted)"
@@ -655,6 +712,9 @@ dry_run_diff() {
     rm -f "$cand"; CAND=""
     die "candidate does not parse as valid YAML — this was a --dry-run, nothing would have been installed either"
   fi
+  if [ "$MODE" = "set" ]; then
+    warn_set_reformat "$CONFIG" "$cand"
+  fi
   say "dry run — diff (redacted), nothing installed"
   diff -u "$CONFIG" "$cand" | redact || true
   rm -f "$cand"; CAND=""

scripts/test-config-edit.sh

@@ -545,6 +545,57 @@ test_refusal_shape_from_parse_failure_wording_is_recognised() {
   assert_equals 4 "$RUN_RC" "the parse-failure refusal shape must also exit 4, not be read as silence"
 }
 
+# --set runs yq over the whole candidate. It warns when that changes more lines than the requested
+# pairs, but a simple file with only the intended changed line must stay quiet.
+new_fixture_reformat_sensitive() {
+  local dir
+  dir="$(mktemp -d "$TMP/fixture.XXXXXX")"
+  cat > "$dir/fleetd.yaml" <<'YAML'
+# A section comment that documents the next block.
+bind:
+  host: 127.0.0.1 # Keep this aligned with the port note.
+  port: 19999     # A fixture port.
+
+# These comments use their placement as documentation.
+profiles:
+  sonnet:
+    weight: 3
+    bootstrapText: >-
+      First line.
+      Second line.
+YAML
+  : > "$dir/fleetd.out"
+  printf '%s' "$dir"
+}
+
+test_set_warns_when_yq_reformats_extra_lines() {
+  local dir
+  dir="$(new_fixture_reformat_sensitive)"
+
+  start_run "$dir" 5 --set '.profiles.sonnet.weight=4'
+  sleep 1
+  printf 'config reloaded\n' >> "$dir/fleetd.out"
+  collect_run "$dir"
+
+  assert_equals 0 "$RUN_RC" "reformat warning case reload exit code"
+  assert_contains "yq reformatted the whole file" "$RUN_OUTPUT" \
+    "a --set that changes extra candidate lines must warn before installation"
+}
+
+test_set_stays_quiet_without_formatting_churn() {
+  local dir
+  dir="$(new_fixture)"
+
+  start_run "$dir" 5 --set '.profiles.sonnet.weight=4'
+  sleep 1
+  printf 'config reloaded\n' >> "$dir/fleetd.out"
+  collect_run "$dir"
+
+  assert_equals 0 "$RUN_RC" "no-reformat warning case reload exit code"
+  assert_not_contains "yq reformatted the whole file" "$RUN_OUTPUT" \
+    "a --set that changes only its requested candidate line must not warn"
+}
+
 echo "== acceptance criterion 1: refusal restores byte for byte =="
 test_refusal_restores_byte_for_byte
 echo "== acceptance criterion 2: clean reload keeps the edit =="
@@ -581,5 +632,9 @@ echo "== extra: --check is read-only and always exits 0 =="
 test_check_is_read_only_and_exits_zero
 echo "== extra: the parse-failure refusal shape is also recognised =="
 test_refusal_shape_from_parse_failure_wording_is_recognised
+echo "== acceptance criterion 17: --set warns about yq formatting churn =="
+test_set_warns_when_yq_reformats_extra_lines
+echo "== acceptance criterion 18: --set stays quiet without formatting churn =="
+test_set_stays_quiet_without_formatting_churn
 
 printf 'PASS: config-edit acceptance criteria\n'