#400: classify the blanking loop's result on the observed value, not eval's exit status

eval "export NAME=" can return success even when zsh coerces the bare
assignment on an integer special parameter (SECONDS, RANDOM, SHLVL,
HISTSIZE, COLUMNS, LINES, USERNAME) instead of failing, leaving the
value unchanged. The old exit-status check then reported the name as
blanked when it was not -- a false receipt.

Classify on the observed effect instead: attempt the export, then read
the name's value back with the (P) indirection flag and decide from
whether it is now empty. One check now covers all three shapes a name
can take here -- a genuine blank, a fatal read-only error eval merely
contains, and this silent no-op -- with the exit status playing no
part in the decision.

Adds a test driving all three shapes through the real scrubScript in
one run (a normal name, LINENO for the fatal case, SECONDS for the
silent no-op), with the parent environment explicitly carrying those
names since a cleared ProcessBuilder parent does not expose them on
its own. Also corrects the previously-merged
unblankableNameInTheMiddleDoesNotAbortNamesAfterIt test, whose "exactly
one failed name" assertion turned out to only pass by accident: zsh
itself auto-exports SHLVL on every shell start, and the old exit-status
bug was silently miscounting it as blanked. The fixed classification
now correctly reports it unblankable too, so the test asserts presence
rather than an exact count.

fleetd/src/main/java/dev/ltms/fleet/member/EnvAllowListScrub.java

@@ -97,6 +97,17 @@ import java.util.stream.Stream;
  * different loop, and the two must not silently drift apart. Re-checking right before the
  * {@code eval} keeps that call site safe by its own reading, independent of whatever the enumeration
  * loop does or stops doing in a later change.
+ *
+ * <p><b>fleetd #400:</b> {@code eval}'s exit status is not proof that the blank actually happened.
+ * zsh coerces a bare {@code NAME=} assignment on an integer special parameter (measured on macOS zsh
+ * 5.9: {@code SECONDS}, {@code RANDOM}, {@code SHLVL}, {@code HISTSIZE}, {@code COLUMNS},
+ * {@code LINES}, {@code USERNAME}) to a number instead of failing — {@code eval} returns success,
+ * the value is untouched, and a status-based classification reports it as blanked when it was not.
+ * The fix classifies on the observed effect instead: after the attempt, the name's value is read
+ * back with the {@code (P)} indirection flag and the decision is made from whether that is now
+ * empty. This one check covers all three shapes a name can take at this point — a genuine blank, a
+ * fatal read-only error {@code eval} merely contained, and this silent no-op — and the exit status
+ * plays no part in the decision at all.
  */
 public final class EnvAllowListScrub {
 
@@ -376,6 +387,16 @@ public final class EnvAllowListScrub {
                 # operation this can never fire (the enumeration guard already filtered everything
                 # reaching _cb633_blank), so a name caught here is counted as unblankable rather than
                 # silently dropped — it is real evidence that the upstream guard was bypassed.
+                #
+                # fleetd #400: the attempt's own exit status is NOT proof of its effect. zsh coerces
+                # a bare `NAME=` assignment on an integer special parameter (SECONDS, RANDOM, SHLVL,
+                # HISTSIZE, COLUMNS, LINES, USERNAME on this host) to a number instead of failing —
+                # `eval` returns 0, the value is untouched, and the old exit-status check reported it
+                # as blanked when it was not. Classify on the observed effect instead: attempt the
+                # export, then read the name's value back with the `(P)` indirection flag and decide
+                # from whether it is now empty. One check then covers all three shapes a name can
+                # take here — a genuine blank, a fatal read-only error `eval` merely contained, and
+                # this silent no-op — without the exit status entering the decision at all.
                 typeset -a _cb633_ok _cb633_unblankable
                 _cb633_ok=()
                 _cb633_unblankable=()
@@ -384,7 +405,8 @@ public final class EnvAllowListScrub {
                     _cb633_unblankable+=("$_cb633_n")
                     continue
                   fi
-                  if eval "export ${_cb633_n}=" 2>/dev/null; then
+                  eval "export ${_cb633_n}=" 2>/dev/null
+                  if [[ -z "${(P)_cb633_n}" ]]; then
                     _cb633_ok+=("$_cb633_n")
                   else
                     _cb633_unblankable+=("$_cb633_n")

fleetd/src/test/java/dev/ltms/fleet/member/EnvAllowListScrubTest.java

@@ -183,11 +183,85 @@ class EnvAllowListScrubTest {
                         + "silently left untouched");
         assertTrue(report.blanked().contains("FLEETD_TEST_AFTER_B"),
                 "the SECOND name after the unblankable one must also still be blanked");
-        assertEquals(1, report.failed(),
-                "exactly one attempted name could not be blanked, and that count must be visible "
-                        + "without reading the member's environment");
-        assertEquals(List.of("FLEETD_TEST_UNBLANKABLE"), report.unblankable(),
+        assertTrue(report.unblankable().contains("FLEETD_TEST_UNBLANKABLE"),
                 "the unblankable name is reported by name, not silently dropped");
+        // fleetd #400 note: zsh itself auto-exports SHLVL on every shell start (measured: it appears
+        // in `command env` even from a fully cleared parent), and a bare assignment to it is coerced
+        // rather than failing — exactly the shape #400 fixes. Before that fix, eval's exit status
+        // alone silently misclassified SHLVL as blanked, so this test's old "exactly one" assertion
+        // passed by accident: it never actually proved SHLVL was absent from the candidates, only
+        // that the old bug hid it. Now that classification reads the value back, SHLVL and
+        // FLEETD_TEST_UNBLANKABLE both correctly land in unblankable() — real, unplanted evidence
+        // the #400 fix works, not just the synthetic case in the dedicated #400 test above.
+        assertTrue(report.unblankable().contains("SHLVL"),
+                "fleetd #400: zsh's own auto-exported SHLVL must also be reported unblankable, not "
+                        + "silently miscounted as blanked");
+        assertEquals(report.unblankable().size(), report.failed(),
+                "the failed count must equal the number of names actually reported unblankable");
+    }
+
+    /**
+     * fleetd #400: {@code eval}'s exit status is not proof that a name was actually blanked. zsh
+     * coerces a bare {@code NAME=} assignment on an integer special parameter to a number instead of
+     * failing, so {@code eval} reports success while the value stays non-empty — a status-based
+     * classification calls that "blanked" when it was not. This drives all three shapes a name can
+     * take through the real {@code scrubScript} in ONE run: a normal, genuinely blankable name; a
+     * fatal one ({@code LINENO} — deliberately not {@code UID}, so this test does not depend on the
+     * harness's uid); and the silent-no-op one the ticket is about ({@code SECONDS}, rc 0 but
+     * unchanged). Under the pre-#400 exit-status check, {@code SECONDS} would land in
+     * {@code blanked()} — that is the exact false receipt this fix removes.
+     *
+     * <p>Criterion 3: a cleared {@code ProcessBuilder} parent does not, by itself, give the child
+     * zsh any of these names — {@code SECONDS}/{@code LINENO} are zsh's own built-in parameters and
+     * only become CANDIDATES the enumeration loop can see (i.e. show up in {@code command env}) when
+     * they arrive via the process's own environment table, not merely by existing as zsh parameters
+     * inside the shell. So each is put into {@code pb.environment()} explicitly, after
+     * {@code clear()} — confirmed empirically first (a throwaway probe piping
+     * {@code env -i PATH=... SECONDS=999 LINENO=999 FLEETD_TEST_NORMAL=1 zsh -c 'command env | cut
+     * -d= -f1'}) that all three names really appear in {@code command env}'s output under exactly
+     * this construction, not relying on whatever the test-runner's own ambient environment happens
+     * to contain.
+     */
+    @Test
+    void classifiesByObservedValueNotExitStatusAcrossAllThreeShapes(@TempDir Path tmp) throws Exception {
+        assumeTrue(Files.isExecutable(ZSH), "/bin/zsh not present — nothing to prove here");
+
+        String script = EnvAllowListScrub.scrubScript(Set.of("ZDOTDIR"));
+
+        ProcessBuilder pb = new ProcessBuilder("/bin/zsh");
+        pb.environment().clear();
+        pb.environment().put("PATH", "/usr/bin:/bin");
+        pb.environment().put("ZDOTDIR", tmp.toAbsolutePath().toString());
+        // Explicitly placed in the child's environment table — see the javadoc above on why a
+        // cleared parent alone does not put these on the enumeration loop's candidate list.
+        pb.environment().put("SECONDS", "999");   // rc 0, value coerced/unchanged — the #400 bug
+        pb.environment().put("LINENO", "999");    // fatal on assignment, eval rc != 0, contained
+        pb.environment().put("FLEETD_TEST_NORMAL", "1"); // genuinely blankable, the control case
+        pb.redirectError(ProcessBuilder.Redirect.DISCARD);
+        Process zsh = pb.start();
+        zsh.getOutputStream().write(script.getBytes(StandardCharsets.UTF_8));
+        zsh.getOutputStream().flush();
+        zsh.getOutputStream().close();
+        assertTrue(zsh.waitFor(60, java.util.concurrent.TimeUnit.SECONDS),
+                "the scrub script did not exit within 60s");
+        assertEquals(0, zsh.exitValue(),
+                "the scrub script must still reach its end with both a fatal name and a silent "
+                        + "no-op name among the candidates");
+
+        EnvAllowListScrub.ScrubReport report = EnvAllowListScrub.readReport(tmp);
+        assertNotNull(report, "the report must still be written");
+        assertTrue(report.blanked().contains("FLEETD_TEST_NORMAL"),
+                "the control case: an ordinary name is genuinely blankable and must be reported so");
+        assertTrue(report.unblankable().contains("LINENO"),
+                "a name fatal to assign to must be reported unblankable — sanity check that "
+                        + "containment still works under the new classification");
+        assertTrue(report.unblankable().contains("SECONDS"),
+                "the #400 defect: eval returns rc 0 for SECONDS (zsh coerces the assignment instead "
+                        + "of failing) but the value is left non-empty — classifying on the observed "
+                        + "value catches this; classifying on eval's exit status would have called "
+                        + "this \"blanked\" and produced a false receipt");
+        assertFalse(report.blanked().contains("SECONDS"),
+                "SECONDS must never appear as blanked — it was never actually emptied");
     }
 
     /**