fleetd #492 follow-up: carry the unclear detail across detect_supervisor's subshell boundary

b17f37a set SUPERVISOR_UNCLEAR_DETAIL as a global inside detect_supervisor, but the real call
site invokes it as $(detect_supervisor) — a subshell — so that global died with the subshell and
the die() message's ${VAR:-fallback} silently masked the loss with generic text.

- detect_supervisor now packs kind and detail onto its one stdout line (joined by the ASCII unit
  separator byte, $SUPERVISOR_DETAIL_SEP), the only channel that survives $( ). The real call site
  unpacks both with in-shell parameter expansion — no extra subshell.
- Dropped the ${SUPERVISOR_UNCLEAR_DETAIL:-...} fallback at the die() message: under set -u, a
  missing detail now fails loudly instead of silently defaulting (same defect class as #497).
- Added a constraints comment block above detect_supervisor for future callers: stdout-only,
  no ${VAR:-default} papering over a lost value, and every case on the return value needs an
  explicit *) arm.
- Added *) arms to the three `case "$SUPERVISOR_KIND"` switches (report/stop/start): report warns
  and continues (display-only), stop/start die naming the value (they act on it).
- Rewrote the "unclear" test to go through the real call-site shape ($(detect_supervisor) then
  the same split), not a hand-constructed value, and tightened its final assertion to check for
  the actual detail text rather than $SYSTEMD_UNIT alone (the die() boilerplate names the unit
  either way, so that check could pass on a lost value).

scripts/redeploy-fleetd.sh

@@ -75,6 +75,12 @@ LAUNCHD_PLIST="$HOME/Library/LaunchAgents/$LAUNCHD_LABEL.plist"
 # "dev.ltms.fleetd" — systemd user units here are not namespaced the way the launchd label is).
 SYSTEMD_UNIT='fleetd'
 
+# fleetd #492 follow-up: detect_supervisor packs TWO values (kind, detail) onto the one stdout
+# line that survives its $(...) call — see the constraints comment above that function. This is
+# the separator between them: the ASCII "unit separator" byte, chosen because it never occurs in
+# any of the prose detail strings and needs no escaping in a `case`/glob pattern.
+SUPERVISOR_DETAIL_SEP=$'\x1f'
+
 # fleetd #492 follow-up: set by systemd_loaded/systemd_installed when the underlying `systemctl`
 # call could not answer cleanly — it exited non-zero AND wrote something to stderr, which is a real
 # tool failure (e.g. it cannot reach the user bus over a non-lingering ssh session), never the same
@@ -84,10 +90,14 @@ SYSTEMD_UNIT='fleetd'
 # reads a deterministic 0 rather than whatever a previous probe call left behind.
 SYSTEMD_LOADED_ERRORED=0
 SYSTEMD_INSTALLED_ERRORED=0
-# fleetd #492 follow-up: set by detect_supervisor alongside an "unclear" answer, naming the specific
+# fleetd #492 follow-up: SUPERVISOR_UNCLEAR_DETAIL is the specific supervisor/reason that
-# supervisor and reason so require_drivable_supervisor's die() message is not just the bare word
+# require_drivable_supervisor's die() names on an "unclear" answer. Deliberately NOT pre-declared
-# "unclear". Initialized empty for the same `set -u` reason as above.
+# here (unlike the two flags above): it is set only by the real call site, right after it unpacks
-SUPERVISOR_UNCLEAR_DETAIL=""
+# detect_supervisor's stdout (see the constraints comment above detect_supervisor). If that call
+# site is ever skipped or broken, a bare `set -u` reference to this variable in
+# require_drivable_supervisor must fail loudly with "unbound variable" — a pre-declared empty
+# default would instead silently print an empty reason, hiding exactly the value this ticket
+# exists to surface.
 
 DO_BUILD=1; ASSUME_YES=0; CHECK_ONLY=0
 for arg in "$@"; do
@@ -187,17 +197,33 @@ systemd_loaded() {
 #     message — a real tool failure, e.g. it cannot reach the user bus over a non-lingering ssh
 #     session — never conflated with a clean negative answer.
 # "none" now means only: neither supervisor is installed, neither is loaded, and neither probe
-# errored. SUPERVISOR_UNCLEAR_DETAIL is set alongside "unclear" so require_drivable_supervisor's
+# errored.
-# die() can name the specific supervisor and reason, not just the bare word "unclear".
 #
-# Pure and side-effect-free besides setting the two globals above: reads the four probes and
+# fleetd #492 follow-up — constraints every caller of this function depends on (learned the hard
-# decides — never mutates anything, so it is safe under --check and testable by overriding
+# way: an earlier version of this fix set a SUPERVISOR_UNCLEAR_DETAIL global from inside here and
+# it was silently lost, because every real call site invokes this as `$(detect_supervisor)`):
+#   1. It is called as `$(detect_supervisor)`, so ONLY STDOUT crosses back to the caller. Anything
+#      this function needs to tell its caller — the "unclear" detail included — must be printed,
+#      never assigned to a global: a global set inside a `$( )` subshell dies with that subshell.
+#      This function packs BOTH values (kind and detail) onto that one stdout line, joined by
+#      $SUPERVISOR_DETAIL_SEP, and the caller unpacks them on its own side of the subshell boundary.
+#   2. This script runs under `set -euo pipefail` (line 50), so an unset variable is a loud
+#      failure. Do not add a `${VAR:-default}` anywhere downstream to paper over a value that
+#      should always be there — that hides a lost value instead of surfacing it (fleetd #497's
+#      defect class).
+#   3. Every `case` on this function's return value needs an explicit final `*)` arm, chosen by
+#      whether that caller ACTS on the value (`die` — an unrecognised value must never be silently
+#      driven) or only DISPLAYS it (`echo`/`warn` and continue — a diagnostic must not go silent on
+#      exactly the value it most needs to report).
+#
+# Pure and side-effect-free besides the two *_ERRORED flags (read back within this same call, never
+# by the caller — see the constraints above): reads the four probes and decides — never mutates
+# anything, so it is safe under --check and testable by overriding
 # launchd_installed/launchd_loaded/systemd_installed/systemd_loaded after sourcing.
 detect_supervisor() {
-  local ld=0 sd=0 li=0 si=0
+  local ld=0 sd=0 li=0 si=0 kind detail=""
   SYSTEMD_LOADED_ERRORED=0
   SYSTEMD_INSTALLED_ERRORED=0
-  SUPERVISOR_UNCLEAR_DETAIL=""
 
   launchd_loaded && ld=1
   systemd_loaded && sd=1
@@ -205,23 +231,25 @@ detect_supervisor() {
   systemd_installed && si=1
 
   if [ "$SYSTEMD_LOADED_ERRORED" = 1 ] || [ "$SYSTEMD_INSTALLED_ERRORED" = 1 ]; then
-    SUPERVISOR_UNCLEAR_DETAIL="the systemd --user probe for '$SYSTEMD_UNIT' could not answer cleanly (systemctl exited non-zero and reported an error on stderr, not a clean negative — e.g. it cannot reach the user bus)"
+    detail="the systemd --user probe for '$SYSTEMD_UNIT' could not answer cleanly (systemctl exited non-zero and reported an error on stderr, not a clean negative — e.g. it cannot reach the user bus)"
-    echo "unclear"
+    kind="unclear"
   elif [ "$ld" = 1 ] && [ "$sd" = 1 ]; then
-    echo "ambiguous"
+    kind="ambiguous"
   elif [ "$li" = 1 ] && [ "$ld" = 0 ]; then
-    SUPERVISOR_UNCLEAR_DETAIL="the launchd agent ($LAUNCHD_LABEL) is installed ($LAUNCHD_PLIST exists) but is not currently loaded"
+    detail="the launchd agent ($LAUNCHD_LABEL) is installed ($LAUNCHD_PLIST exists) but is not currently loaded"
-    echo "unclear"
+    kind="unclear"
   elif [ "$si" = 1 ] && [ "$sd" = 0 ]; then
-    SUPERVISOR_UNCLEAR_DETAIL="the systemd --user unit ($SYSTEMD_UNIT) is installed but not currently active — it may be activating, deactivating, failed, or waiting on an auto-restart"
+    detail="the systemd --user unit ($SYSTEMD_UNIT) is installed but not currently active — it may be activating, deactivating, failed, or waiting on an auto-restart"
-    echo "unclear"
+    kind="unclear"
   elif [ "$ld" = 1 ]; then
-    echo "launchd"
+    kind="launchd"
   elif [ "$sd" = 1 ]; then
-    echo "systemd"
+    kind="systemd"
   else
-    echo "none"
+    kind="none"
   fi
+
+  printf '%s%s%s' "$kind" "$SUPERVISOR_DETAIL_SEP" "$detail"
 }
 
 # fleetd #492: turns anything detect_supervisor returns that is NOT exactly one of the two
@@ -240,16 +268,18 @@ require_drivable_supervisor() {
           prevent. Stop one of the two supervisors by hand, confirm only one remains loaded, then
           rerun." ;;
     unclear)
-      # fleetd #492 follow-up: SUPERVISOR_UNCLEAR_DETAIL is set by detect_supervisor right before
+      # fleetd #492 follow-up: SUPERVISOR_UNCLEAR_DETAIL crosses back from detect_supervisor's
-      # it returns "unclear"; the fallback text below only fires if this is ever called directly
+      # subshell via its stdout, unpacked by the caller BEFORE it calls this function (see the
-      # (as a test does) without going through detect_supervisor first.
+      # constraints comment above detect_supervisor). No ${VAR:-default} here on purpose: if the
+      # detail is somehow missing, `set -u` makes this reference fail loudly instead of silently
+      # naming nothing — a default that hides a lost value is the same defect class as fleetd
+      # #497.
       die "a supervisor looks present but this script cannot tell whether it actually drives this
-          daemon: ${SUPERVISOR_UNCLEAR_DETAIL:-launchd ($LAUNCHD_LABEL) or systemd --user
+          daemon: $SUPERVISOR_UNCLEAR_DETAIL. Guessing wrong here is the same failure 'ambiguous'
-          ($SYSTEMD_UNIT) reported something other than a clean 'loaded' or a clean 'not loaded'}.
+          above exists to prevent: driving the daemon while an unseen supervisor revives the OLD
-          Guessing wrong here is the same failure 'ambiguous' above exists to prevent: driving the
+          jar out from under it (fleetd #492). Check 'launchctl list $LAUNCHD_LABEL' and
-          daemon while an unseen supervisor revives the OLD jar out from under it (fleetd #492).
+          'systemctl --user status $SYSTEMD_UNIT' by hand, resolve whichever looks unclear, then
-          Check 'launchctl list $LAUNCHD_LABEL' and 'systemctl --user status $SYSTEMD_UNIT' by
+          rerun." ;;
-          hand, resolve whichever looks unclear, then rerun." ;;
     *)
       die "detect_supervisor returned an unrecognized value '$kind' — refusing to guess which
           supervisor, if any, controls this daemon." ;;
@@ -401,7 +431,12 @@ fi
 # outright — before touching anything — if that cannot be told apart (see require_drivable_
 # supervisor above). --check reaches this same line, so a host with an undrivable supervisor is
 # reported as a failure even in --check, without ever reaching the build/stop/start steps.
-SUPERVISOR_KIND="$(detect_supervisor)"
+# fleetd #492 follow-up: detect_supervisor runs as $(...), so only the printed line survives —
+# unpack kind and detail from it HERE, in this shell, before calling anything downstream. See the
+# constraints comment above detect_supervisor for why this cannot be done any other way.
+SUPERVISOR_RAW="$(detect_supervisor)"
+SUPERVISOR_KIND="${SUPERVISOR_RAW%%"$SUPERVISOR_DETAIL_SEP"*}"
+SUPERVISOR_UNCLEAR_DETAIL="${SUPERVISOR_RAW#*"$SUPERVISOR_DETAIL_SEP"}"
 require_drivable_supervisor "$SUPERVISOR_KIND"
 ok "supervisor detected: $SUPERVISOR_KIND"
 SUPERVISED=0
@@ -420,6 +455,13 @@ case "$SUPERVISOR_KIND" in
   none)
     warn "no supervisor loaded — this script is the only thing that will restart the daemon."
     ;;
+  *)
+    # fleetd #492 follow-up: this block only DISPLAYS state, it changes nothing yet — so a value
+    # it doesn't recognise gets reported, not an abort that goes silent on exactly the state most
+    # worth seeing. (Unreachable today: require_drivable_supervisor above already died on
+    # "ambiguous"/"unclear" before this case runs. Guards the value nobody has invented yet.)
+    warn "unrecognised supervisor kind: '$SUPERVISOR_KIND' — detect_supervisor returned a value this block does not know; continuing to report the rest of the state."
+    ;;
 esac
 
 # The trap with no log line. Checked in a LOGIN shell, because that is how the daemon is started
@@ -534,6 +576,14 @@ if [ -n "$OLD_PID" ]; then
     none)
       kill "$OLD_PID"
       ;;
+    *)
+      # fleetd #492 follow-up: this block ACTS (stops the daemon one specific way per kind) — an
+      # unrecognised value must never fall through to a default action, silently picking the wrong
+      # one (or none at all) while reporting success. (Unreachable today: require_drivable_
+      # supervisor already died before this runs. Guards the value nobody has invented yet.)
+      die "detect_supervisor returned an unrecognized value '$SUPERVISOR_KIND' at the stop step —
+          refusing to guess how to stop a daemon under an unknown supervisor. The daemon was NOT
+          stopped." ;;
   esac
   for _ in $(seq "$STOP_WAIT"); do
     [ -z "$(running_pid)" ] && break
@@ -607,6 +657,14 @@ case "$SUPERVISOR_KIND" in
     # Absolute jar path so `ps` names which checkout is running.
     ( cd "$MODULE" && zsh -lc "nohup java -jar '$JAR' >> fleetd.out 2>&1 &" )
     ;;
+  *)
+    # fleetd #492 follow-up: this block ACTS (starts the daemon one specific way per kind) — an
+    # unrecognised value must never fall through to a default action, silently picking the wrong
+    # one (or none at all) while reporting success. (Unreachable today: require_drivable_
+    # supervisor already died before this runs. Guards the value nobody has invented yet.)
+    die "detect_supervisor returned an unrecognized value '$SUPERVISOR_KIND' at the start step —
+        refusing to guess how to start a daemon under an unknown supervisor. The daemon was NOT
+        started." ;;
 esac
 
 for _ in $(seq 10); do

scripts/test-redeploy-fleetd.sh

@@ -25,6 +25,17 @@ classify_fixture() {
   classify_amqp_connection_errors "$TMP/$name"
 }
 
+# fleetd #492 follow-up: detect_supervisor's stdout is now "kind<SEP>detail" (see the constraints
+# comment above detect_supervisor in redeploy-fleetd.sh) — every test below that only cares about
+# the kind must split it out with the SAME in-shell parameter expansion the real call site (:438)
+# uses, never a bare string comparison against the raw output.
+supervisor_kind_of() {
+  printf '%s' "${1%%"$SUPERVISOR_DETAIL_SEP"*}"
+}
+supervisor_detail_of() {
+  printf '%s' "${1#*"$SUPERVISOR_DETAIL_SEP"}"
+}
+
 
 # fleetd #492 — supervisor detection. Detect_supervisor() reads launchd_loaded/systemd_loaded, so
 # each test overrides BOTH pairs (installed + loaded) explicitly, rather than relying on either
@@ -37,7 +48,7 @@ test_detect_supervisor_launchd_only() {
   launchd_loaded()    { return 0; }
   systemd_installed() { return 1; }
   systemd_loaded()    { return 1; }
-  assert_equals "launchd" "$(detect_supervisor)" "launchd-only detection"
+  assert_equals "launchd" "$(supervisor_kind_of "$(detect_supervisor)")" "launchd-only detection"
 }
 
 test_detect_supervisor_systemd_only() {
@@ -45,7 +56,7 @@ test_detect_supervisor_systemd_only() {
   launchd_loaded()    { return 1; }
   systemd_installed() { return 0; }
   systemd_loaded()    { return 0; }
-  assert_equals "systemd" "$(detect_supervisor)" "systemd-only detection"
+  assert_equals "systemd" "$(supervisor_kind_of "$(detect_supervisor)")" "systemd-only detection"
 }
 
 test_detect_supervisor_none() {
@@ -53,7 +64,7 @@ test_detect_supervisor_none() {
   launchd_loaded()    { return 1; }
   systemd_installed() { return 1; }
   systemd_loaded()    { return 1; }
-  assert_equals "none" "$(detect_supervisor)" "unsupervised detection"
+  assert_equals "none" "$(supervisor_kind_of "$(detect_supervisor)")" "unsupervised detection"
 }
 
 # fleetd #492 follow-up — detect_supervisor must never answer "none" when the truth is "could not
@@ -66,7 +77,11 @@ test_detect_supervisor_systemd_installed_not_loaded_is_unclear() {
   launchd_loaded()    { return 1; }
   systemd_installed() { return 0; }   # the unit file IS there
   systemd_loaded()    { return 1; }   # is-active says no — could be activating/failed/pending restart
-  assert_equals "unclear" "$(detect_supervisor)" "systemd installed-but-not-loaded must read as unclear, not none"
+  local raw
+  raw="$(detect_supervisor)"
+  assert_equals "unclear" "$(supervisor_kind_of "$raw")" "systemd installed-but-not-loaded must read as unclear, not none"
+  printf '%s' "$(supervisor_detail_of "$raw")" | grep -qF "$SYSTEMD_UNIT" \
+    || fail "detail does not name the systemd unit it found installed-but-not-loaded"
 }
 
 # Same fact, the launchd side: a plist on disk that is not currently loaded (unloaded without being
@@ -76,7 +91,11 @@ test_detect_supervisor_launchd_installed_not_loaded_is_unclear() {
   launchd_loaded()    { return 1; }   # launchctl list says not loaded
   systemd_installed() { return 1; }
   systemd_loaded()    { return 1; }
-  assert_equals "unclear" "$(detect_supervisor)" "launchd installed-but-not-loaded must read as unclear, not none"
+  local raw
+  raw="$(detect_supervisor)"
+  assert_equals "unclear" "$(supervisor_kind_of "$raw")" "launchd installed-but-not-loaded must read as unclear, not none"
+  printf '%s' "$(supervisor_detail_of "$raw")" | grep -qF "$LAUNCHD_LABEL" \
+    || fail "detail does not name the launchd label it found installed-but-not-loaded"
 }
 
 # Drives the REAL systemd_loaded/systemd_installed bodies (never stubbed) through a `systemctl`
@@ -108,24 +127,42 @@ STUB
   launchd_installed() { return 1; }
   launchd_loaded()    { return 1; }
   result="$(PATH="$bin_dir:$PATH" detect_supervisor)"
-  assert_equals "unclear" "$result" "a systemd probe error must read as unclear, not none"
+  assert_equals "unclear" "$(supervisor_kind_of "$result")" "a systemd probe error must read as unclear, not none"
+  printf '%s' "$(supervisor_detail_of "$result")" | grep -qF "$SYSTEMD_UNIT" \
+    || fail "detail does not name the systemd unit whose probe errored"
 }
 
-# The other half of the ticket: an "unclear" supervisor must refuse exactly like "ambiguous" does —
+# fleetd #492 follow-up (Item 1): this must go through the REAL call-site shape at :437-440, not a
-# die(), never fall through to the `kill` path — and the refusal must name the specific supervisor
+# hand-constructed "unclear" value — a test that builds "unclear" directly proves the switch, not
-# and reason detect_supervisor found, not just the bare word "unclear".
+# the handoff, and that is exactly the gap that let SUPERVISOR_UNCLEAR_DETAIL never reach the real
+# caller in b17f37a. detect_supervisor runs as $(detect_supervisor): a subshell. Only stdout
+# survives that boundary, so kind AND detail must both cross on it — this test proves they do.
 test_require_drivable_supervisor_refuses_unclear() {
   launchd_installed() { return 1; }
   launchd_loaded()    { return 1; }
   systemd_installed() { return 0; }
   systemd_loaded()    { return 1; }
-  local kind output rc=0
+
-  kind="$(detect_supervisor)"
+  local SUPERVISOR_RAW SUPERVISOR_KIND SUPERVISOR_UNCLEAR_DETAIL output rc=0
-  assert_equals "unclear" "$kind" "setup: expected unclear before testing the refusal"
+  # Exactly what :437-439 does — do not shortcut this by constructing "unclear" by hand.
-  output="$(require_drivable_supervisor "$kind" 2>&1)" || rc=$?
+  SUPERVISOR_RAW="$(detect_supervisor)"
+  SUPERVISOR_KIND="${SUPERVISOR_RAW%%"$SUPERVISOR_DETAIL_SEP"*}"
+  SUPERVISOR_UNCLEAR_DETAIL="${SUPERVISOR_RAW#*"$SUPERVISOR_DETAIL_SEP"}"
+
+  assert_equals "unclear" "$SUPERVISOR_KIND" "setup: expected unclear before testing the refusal"
+  [ -n "$SUPERVISOR_UNCLEAR_DETAIL" ] \
+    || fail "detail did not survive the \$(...) call-site boundary — SUPERVISOR_UNCLEAR_DETAIL is empty in the parent shell"
+  printf '%s' "$SUPERVISOR_UNCLEAR_DETAIL" | grep -qF "$SYSTEMD_UNIT" \
+    || fail "detail that crossed the subshell boundary does not name the systemd unit it found installed-but-not-loaded"
+
+  output="$(require_drivable_supervisor "$SUPERVISOR_KIND" 2>&1)" || rc=$?
   [ "$rc" -ne 0 ] || fail "require_drivable_supervisor accepted an unclear (undrivable) supervisor"
-  printf '%s' "$output" | grep -qF "$SYSTEMD_UNIT" \
+  # Check for the ACTUAL DETAIL TEXT, not just "$SYSTEMD_UNIT" — the die() message's boilerplate
-    || fail "refusal message does not name the systemd unit it found installed-but-not-loaded"
+  # recovery instructions name the unit unconditionally either way ("systemctl --user status
+  # $SYSTEMD_UNIT"), so a bare unit-name grep here would pass even on a lost/fallback detail. Only
+  # the specific detail string proves the crossed value, not the boilerplate, reached the message.
+  printf '%s' "$output" | grep -qF "$SUPERVISOR_UNCLEAR_DETAIL" \
+    || fail "refusal message does not contain the specific detail that crossed the subshell boundary"
 }
 
 # The heart of the ticket: a supervisor this script cannot drive must refuse, never fall through to