#400: classify the blanking loop on the observed value, not eval exit status #403

2026-09-10T03:47:09+02:00

agent commented

2026-09-10 03:47:09 +02:00

Fixes fleetd #400: a defect in the just-merged #394/#397 blanking loop.

The bug

The blanking loop classified a name as blanked (_cb633_ok) or unblankable
(_cb633_unblankable) based on eval's exit status. zsh coerces a bare
NAME= assignment on an integer special parameter (measured: SECONDS,
RANDOM, SHLVL, HISTSIZE, COLUMNS, LINES, USERNAME) to a number
instead of failing, so eval returns success while the value is left
unchanged. The old check then reported the name as blanked when it was not
— a false receipt. (Today's exposure is nil: none of these names hold a
secret. The receipt's whole purpose was still being violated.)

The fix

Classify on the observed effect instead of the attempt's exit status: after
the eval attempt, 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 — 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. The identifier guard immediately before eval (added
in the #394 follow-up) is unchanged.

Tests

New: classifiesByObservedValueNotExitStatusAcrossAllThreeShapes drives
all three shapes through the real scrubScript() output under a real
/bin/zsh in one run — a normal blankable name (FLEETD_TEST_NORMAL), a
fatal name (LINENO, not UID, so the test does not depend on the
harness's uid), and the rc-0-but-unchanged name (SECONDS). The parent
ProcessBuilder environment is cleared and then explicitly given
SECONDS/LINENO/FLEETD_TEST_NORMAL — confirmed by a throwaway probe
first (env -i PATH=... SECONDS=999 LINENO=999 FLEETD_TEST_NORMAL=1 zsh -c 'command env | cut -d= -f1') that a cleared parent does NOT expose these
names on its own; they only become enumeration candidates because they
are put into the process's own environment table.
Updated the previously-merged unblankableNameInTheMiddleDoesNotAbortNamesAfterIt:
its "exactly one failed name" assertion turned out to pass only by
accident. zsh itself auto-exports SHLVL on every shell start (confirmed:
env -i PATH=/usr/bin:/bin zsh -c 'command env | cut -d= -f1' lists
SHLVL even from a fully cleared parent), and the old exit-status bug was
silently miscounting it as blanked. The fixed classification now correctly
reports SHLVL unblankable too — real, unplanted evidence the fix works,
independent of the dedicated #400 test. The assertion now checks presence
in unblankable() rather than an exact list/count, since which auto-set
names a given zsh version exports is not something this test should pin.

Mutation check (acceptance criterion 4)

Reverted the classification to the old if eval ...; then ok; else unblankable; fi predicate. The new test fails with:

org.opentest4j.AssertionFailedError: 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 ==> expected: <true> but was: <false>

Restored the fix; both the new test and
unblankableNameInTheMiddleDoesNotAbortNamesAfterIt pass again.

Build

mvn clean install, unpiped, full output read:

Tests run: 1472, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

(EnvAllowListScrubTest alone: Tests run: 11, Failures: 0, Errors: 0, Skipped: 0.)

Report back

Criterion 3 (parent must genuinely contain the names): confirmed with
a throwaway probe before writing the test — env -i PATH=/usr/bin:/bin SECONDS=999 LINENO=999 FLEETD_TEST_NORMAL=1 zsh -c 'command env | cut -d= -f1' shows all three names present. A fully cleared parent with only
PATH set shows none of them (only zsh's own auto-set HOME, LOGNAME,
OLDPWD, PWD, SHLVL, _) — so the test's pb.environment().clear()
is followed by explicit .put("SECONDS", ...) / .put("LINENO", ...) /
.put("FLEETD_TEST_NORMAL", ...) rather than relying on inheritance.
${(P)name} is the right mechanism for reading a name's value back
indirectly here — confirmed empirically against the ticket's own reported
numbers before writing any test, and it never requires re-splicing the
name into a string the way a second eval would. No safer/simpler
alternative was found. Only emptiness was ever checked — no value was
printed anywhere in this work.
What else has this shape (exit status trusted as proof of effect,
where the effect could be checked directly) — reported separately below,
not fixed here.

Scope: EnvAllowListScrub.java and its test, nothing else touched.

Fixes fleetd #400: a defect in the just-merged #394/#397 blanking loop. ## The bug The blanking loop classified a name as blanked (`_cb633_ok`) or unblankable (`_cb633_unblankable`) based on `eval`'s exit status. zsh coerces a bare `NAME=` assignment on an integer special parameter (measured: `SECONDS`, `RANDOM`, `SHLVL`, `HISTSIZE`, `COLUMNS`, `LINES`, `USERNAME`) to a number instead of failing, so `eval` returns success while the value is left unchanged. The old check then reported the name as blanked when it was not — a false receipt. (Today's exposure is nil: none of these names hold a secret. The receipt's whole purpose was still being violated.) ## The fix Classify on the observed effect instead of the attempt's exit status: after the `eval` attempt, 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 — 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. The identifier guard immediately before `eval` (added in the #394 follow-up) is unchanged. ## Tests - New: `classifiesByObservedValueNotExitStatusAcrossAllThreeShapes` drives all three shapes through the real `scrubScript()` output under a real `/bin/zsh` in one run — a normal blankable name (`FLEETD_TEST_NORMAL`), a fatal name (`LINENO`, not `UID`, so the test does not depend on the harness's uid), and the rc-0-but-unchanged name (`SECONDS`). The parent `ProcessBuilder` environment is cleared and then explicitly given `SECONDS`/`LINENO`/`FLEETD_TEST_NORMAL` — confirmed by a throwaway probe first (`env -i PATH=... SECONDS=999 LINENO=999 FLEETD_TEST_NORMAL=1 zsh -c 'command env | cut -d= -f1'`) that a cleared parent does NOT expose these names on its own; they only become enumeration candidates because they are put into the process's own environment table. - Updated the previously-merged `unblankableNameInTheMiddleDoesNotAbortNamesAfterIt`: its "exactly one failed name" assertion turned out to pass only by accident. zsh itself auto-exports `SHLVL` on every shell start (confirmed: `env -i PATH=/usr/bin:/bin zsh -c 'command env | cut -d= -f1'` lists `SHLVL` even from a fully cleared parent), and the old exit-status bug was silently miscounting it as blanked. The fixed classification now correctly reports `SHLVL` unblankable too — real, unplanted evidence the fix works, independent of the dedicated #400 test. The assertion now checks presence in `unblankable()` rather than an exact list/count, since which auto-set names a given zsh version exports is not something this test should pin. ## Mutation check (acceptance criterion 4) Reverted the classification to the old `if eval ...; then ok; else unblankable; fi` predicate. The new test fails with: ``` org.opentest4j.AssertionFailedError: 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 ==> expected: <true> but was: <false> ``` Restored the fix; both the new test and `unblankableNameInTheMiddleDoesNotAbortNamesAfterIt` pass again. ## Build `mvn clean install`, unpiped, full output read: ``` Tests run: 1472, Failures: 0, Errors: 0, Skipped: 0 BUILD SUCCESS ``` (`EnvAllowListScrubTest` alone: `Tests run: 11, Failures: 0, Errors: 0, Skipped: 0`.) ## Report back - **Criterion 3 (parent must genuinely contain the names):** confirmed with a throwaway probe before writing the test — `env -i PATH=/usr/bin:/bin SECONDS=999 LINENO=999 FLEETD_TEST_NORMAL=1 zsh -c 'command env | cut -d= -f1'` shows all three names present. A fully cleared parent with only `PATH` set shows none of them (only zsh's own auto-set `HOME`, `LOGNAME`, `OLDPWD`, `PWD`, `SHLVL`, `_`) — so the test's `pb.environment().clear()` is followed by explicit `.put("SECONDS", ...)` / `.put("LINENO", ...)` / `.put("FLEETD_TEST_NORMAL", ...)` rather than relying on inheritance. - **`${(P)name}` is the right mechanism** for reading a name's value back indirectly here — confirmed empirically against the ticket's own reported numbers before writing any test, and it never requires re-splicing the name into a string the way a second `eval` would. No safer/simpler alternative was found. Only emptiness was ever checked — no value was printed anywhere in this work. - **What else has this shape** (exit status trusted as proof of effect, where the effect could be checked directly) — reported separately below, not fixed here. Scope: `EnvAllowListScrub.java` and its test, nothing else touched.

agent added 1 commit 2026-09-10 03:47:10 +02:00

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

CI / contract (pull_request) Successful in 54s

Details

CI / build (pull_request) Successful in 2m1s

Details

c670792ffe

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.

ltms commented

2026-09-10 04:03:06 +02:00

Lead review — good work, and one part of it is better than what I asked for

I read the diff rather than the report. The change is minimal and right:

-                  if eval "export ${_cb633_n}=" 2>/dev/null; then
+                  eval "export ${_cb633_n}=" 2>/dev/null
+                  if [[ -z "${(P)_cb633_n}" ]]; then

Exit status is out of the decision entirely, and the ^[A-Za-z_][A-Za-z0-9_]*$ guard at the eval site is untouched — which matters, because that guard is the only thing making the eval safe.

The part I want to single out

Your point (e) is the best thing in this report. unblankableNameInTheMiddleDoesNotAbortNamesAfterIt — a test merged in #394 — started failing under your fix, and you worked out why instead of adjusting it until it went green: zsh auto-exports SHLVL, the old exit-status bug had been miscounting it as blanked all along, and the corrected classification now reports it honestly.

So a merged test's "exactly one failed name" assertion had only ever passed because of the bug it sat next to. That is unplanted, independent evidence the fix works, and it is worth more than the test you were asked to write. Loosening that assertion from an exact count to a presence check is the right call, for the reason you gave: which names a given zsh version auto-exports is not something this test should pin.

Flagging it as outside literal scope, and then fixing it anyway because it is the same file and the same defect, is exactly the judgment I want. Same for point (a) — you checked that the parent environment really carried the names before writing the test, so pb.environment().clear() plus explicit puts is a deliberate choice, not a copy of the old harness. The #394 test's whole problem was that a cleared parent structurally could not contain the trigger.

Verified myself

Reading the diff, not the report: the fix, the preserved guard, and the receipt channel.

One thing I checked that you did not mention: readReport on main already parses a third channel for names that could not be blanked —

if (line.startsWith("!")) {
    unblankable.add(line.substring(1));
} else {
    blanked.add(line);
}

— and the generated script already emits it at :396-398. So an unblankable name has a real place to go and does not have to fall into the allowed count. Your fix lands straight into that channel with no format change needed. The fleet01 lead independently reached the same verify-after-attempt shape but believed that channel did not exist yet and deliberately withheld the extra line to avoid corrupting the parse; I have told them it is already there.

What I still owe this PR

I have not yet run my own mutation on the half you did not touch — my rule is to mutate a different line than the worker's proof covers. The candidate is the !-prefix output line: if removing it leaves the suite green, the unblankable channel is unpinned and a name that could not be blanked would vanish from the receipt silently, which is the same class of defect as #400 itself.

I will report that result here before merging, pass or fail.

Your survey list

Useful, and correctly not acted on. The GitWorktrees entries are the same "trusted the exit code, never read the value back" shape:

:440-441 blanking credential.helper without a --get-all read-back is credential-adjacent and the one I would take first.
:621,982 git update-index --skip-worktree never verified with git ls-files -v matters more than it looks: the project addendum's neutralized-config mechanism depends on that bit actually being set, and a worker that edits such a file is told nothing when it silently is not.
:1055-1058 never reading back refs/wip/<branch> is a snapshot reported as saved without proof — and that is the handover path.
CaffeinateSleepAssertionMechanism:50-84 trusting the process staying alive rather than pmset -g assertions is the same shape one layer out.

I will file these as one ticket for the shape, not five tickets. Naming your confidence ordering, and marking the last two as low-confidence/benign, is what makes the list usable — thank you for not padding it.

## Lead review — good work, and one part of it is better than what I asked for I read the diff rather than the report. The change is minimal and right: ```zsh - if eval "export ${_cb633_n}=" 2>/dev/null; then + eval "export ${_cb633_n}=" 2>/dev/null + if [[ -z "${(P)_cb633_n}" ]]; then ``` Exit status is out of the decision entirely, and the `^[A-Za-z_][A-Za-z0-9_]*$` guard at the eval site is untouched — which matters, because that guard is the only thing making the `eval` safe. ## The part I want to single out Your point (e) is the best thing in this report. `unblankableNameInTheMiddleDoesNotAbortNamesAfterIt` — a test merged in #394 — started failing under your fix, and you worked out **why** instead of adjusting it until it went green: zsh auto-exports `SHLVL`, the old exit-status bug had been miscounting it as blanked all along, and the corrected classification now reports it honestly. So a merged test's "exactly one failed name" assertion had only ever passed **because of the bug it sat next to.** That is unplanted, independent evidence the fix works, and it is worth more than the test you were asked to write. Loosening that assertion from an exact count to a presence check is the right call, for the reason you gave: which names a given zsh version auto-exports is not something this test should pin. Flagging it as outside literal scope, and then fixing it anyway because it is the same file and the same defect, is exactly the judgment I want. Same for point (a) — you checked that the parent environment really carried the names *before* writing the test, so `pb.environment().clear()` plus explicit `put`s is a deliberate choice, not a copy of the old harness. The #394 test's whole problem was that a cleared parent structurally could not contain the trigger. ## Verified myself Reading the diff, not the report: the fix, the preserved guard, and the receipt channel. One thing I checked that you did not mention: `readReport` on `main` **already** parses a third channel for names that could not be blanked — ```java if (line.startsWith("!")) { unblankable.add(line.substring(1)); } else { blanked.add(line); } ``` — and the generated script already emits it at `:396-398`. So an unblankable name has a real place to go and does not have to fall into the allowed count. Your fix lands straight into that channel with no format change needed. The fleet01 lead independently reached the same verify-after-attempt shape but believed that channel did not exist yet and deliberately withheld the extra line to avoid corrupting the parse; I have told them it is already there. ## What I still owe this PR I have not yet run my own mutation on the half you did not touch — my rule is to mutate a different line than the worker's proof covers. The candidate is the `!`-prefix output line: if removing it leaves the suite green, the unblankable channel is unpinned and a name that could not be blanked would vanish from the receipt silently, which is the same class of defect as #400 itself. I will report that result here before merging, pass or fail. ## Your survey list Useful, and correctly not acted on. The `GitWorktrees` entries are the same "trusted the exit code, never read the value back" shape: - `:440-441` blanking `credential.helper` without a `--get-all` read-back is credential-adjacent and the one I would take first. - `:621,982` `git update-index --skip-worktree` never verified with `git ls-files -v` matters more than it looks: the project addendum's neutralized-config mechanism depends on that bit actually being set, and a worker that edits such a file is told nothing when it silently is not. - `:1055-1058` never reading back `refs/wip/<branch>` is a snapshot reported as saved without proof — and that is the handover path. - `CaffeinateSleepAssertionMechanism:50-84` trusting the process staying alive rather than `pmset -g assertions` is the same shape one layer out. I will file these as one ticket for the shape, not five tickets. Naming your confidence ordering, and marking the last two as low-confidence/benign, is what makes the list usable — thank you for not padding it.

ltms referenced this pull request

2026-09-10 04:03:53 +02:00

Five places trust a command's exit code as proof of its effect and never read the value back — the #400 shape outside the scrub #408

ltms closed this pull request

2026-09-10 07:16:15 +02:00