Skip to content

test(stack): red the suite when a domain file contributes no assertions (#1400) - #1416

Open
VijitSingh97 wants to merge 2 commits into
develop-v2from
fix/1400-source-stanza-guard
Open

test(stack): red the suite when a domain file contributes no assertions (#1400)#1416
VijitSingh97 wants to merge 2 commits into
develop-v2from
fix/1400-source-stanza-guard

Conversation

@VijitSingh97

Copy link
Copy Markdown
Collaborator

tests/stack/run.sh runs under set -uo pipefail with no -e and computes its verdict from
PASS/FAIL alone. A domain file that fails to source contributes nothing to either counter, so
the suite prints its green line and exits 0 with an entire domain never having run. 25 stanzas were
unchecked. The sharpest case is the last one, test-lifecycle.sh, which sits at run.sh:3832
immediately above the verdict.

This is prevention, not a live bug, and that is measured rather than assumed: no domain file has
a top-level return today. The sweep was run with a positive control first — it fires on a planted
return 0 — so the empty result is evidence rather than a pattern that cannot match.

The change

Each of the 24 domain stanzas becomes:

# shellcheck source=tests/stack/test-x.sh disable=SC2015
_d0=$((PASS + FAIL)) && source "$HERE/test-x.sh" && domain_ran test-x.sh "$_d0" "$?" || domain_ran test-x.sh "$_d0" "$?"

lib.sh's own stanza stays bare on purpose: if lib.sh fails to source, PASS is unbound and the
first guarded stanza's arithmetic aborts the shell under set -u. Probed directly — a script file
exits 1 with PASS: unbound variable. That case already fails loud, not green.

One thing worth stating because it is invisible by reading

The design rests on _d0=$((PASS + FAIL)) returning 0 when the sum is zero, which it does: the
exit status of an assignment is not the value of its arithmetic expansion. The near-identical
arithmetic command form (( PASS + FAIL )) returns 1 on a zero result. Written that way, the
very first stanza would short-circuit past its own source on every clean run. It would fail closed
— a loud false RED naming the file, not a silent skip — but the distinction does not survive a
reading, so it is written down.

Why a positive control and not source x || domain_fail x

The issue proposes the status form. It answers a different question: source returns the status of
the file's LAST command. Measured on five fixtures under run.sh's real options, with a healthy
control:

fixture                       DECLINED (|| domain_fail)   SHIPPED (delta)
H healthy (CONTROL)           GREEN                       GREEN
R top-level `return 0`        GREEN  <- misses it         RED
M missing file                RED                         RED
C healthy, last cmd fails     RED  <- false positive      GREEN
P parse error AFTER asserts   RED                         GREEN  <- shipped misses it

Neither form dominates. The shipped form wins row R, the only failure mode that leaves no
diagnostic anywhere, and carries no false-positive surface. The declined form wins row P — but its
extra catch is inseparable from its false positive on row C, because both are just "the last command
returned non-zero". Row H green in BOTH columns is what makes this a strictly better instrument on
the rows that matter rather than a blanket inversion that reds more and looks stricter.

Reviewer call, offered rather than decided silently. A combined delta == 0 || rc != 0 check
would catch strictly more, and its false-positive surface is measurably EMPTY today — an instrumented
full-suite run recorded every one of the 24 domains returning source rc 0, including all four files
that end in rm -f. I did not ship it because it invents an undeclared contract on 24 files ("your
last top-level command must succeed") that the next lane appending a cleanup line would trip with no
way to find out why. Overrule me if you disagree; the measurement is in the repo history of this PR.

Line-neutrality, and the constraint that shaped the form

run.sh is 3841 lines against a 3841 ceiling, so the change had to be line-neutral. The obvious form
_d0=...; source ...; domain_ran ... — is NOT: shfmt -i 4 -d is half of make lint-sh and
rewrites every ;-separated line and every brace group onto separate lines, which takes run.sh to
3889. Measured across candidate forms: a; b; c splits to 3, a; b to 2, { a; b; } to 4;
a && b, a || b and a && b || c are preserved. The line-neutral solution space here is
control-operator chains, not statement separators.

So: 24 directive comments gain disable=SC2015 in place, and 24 source lines are replaced
one-for-one. run.sh 3841 -> 3841. lib.sh 232 -> 259, still under TARGET_LINES, so it stays
unrowed and docs/dev/file-budget.tsv is untouched.

Structural check on the result: all 25 stanzas accounted for — 24 guarded, lib.sh bare, every
disable=SC2015 directive adjacent to its own stanza, and each stanza naming the same filename in
all three positions (directive, source, both domain_ran calls). No mismatches.

On the SC2015 suppression

Both || branches are textually identical because the call must happen either way, and $? carries
source's status correctly into both. domain_ran ends in an explicit return 0, which is
load-bearing rather than decoration: a non-zero return would run the second branch too and count the
same failure twice. SC2015 is the heuristic that cannot see that guarantee.

It is suppressed on the directive comment that was already there, so it costs no lines and is scoped
to exactly these stanzas. It is deliberately NOT left for --severity=warning to filter: shipping 24
findings that pass only because of a severity filter is a shape this repo files.
Verified narrow with a negative control — a fixture whose source target is MISSING still emits
SC1091 and exits 1 under the combined directive, so disable=SC2015 suppresses only SC2015 and the
source= half still resolves. Run at full severity with shellcheck 0.11.0, the version CI pins.

What was RUN

  • After-proof vs baseline, same tip, both real git worktrees: 2922 passed / 0 failed / rc 0 on
    BOTH.
    3233 log lines both, 272 section headers both, same SET and same ORDER (The #1105 split proof recipe cannot see execution reordering — two merged cuts reordered, one unexamined #1398).
    Sorted-multiset diff is exactly 2 lines: the two halves of one ed25519 fingerprint (Two suite verdict lines are nondeterministic, weakening the split-proof multiset check #1325).
  • Negative control: ZERO #1400 lines in the after-proof. The guard does not fire on a healthy
    tree.
  • Positive control: all 24 domains measured with nonzero assertion deltas, 9 to 207. A guard that
    can never fire is not a guard; this one is satisfiable by every domain that exists.
  • make lint-sh rc 0, Makefile's exact pair, PATH set so shellcheck resolves the CI-pinned
    0.11.0. Not banked on its own — it returned in under two minutes, so it was checked with a
    negative control the repo already predicts: drop os/overlay/pithead-boot and run.sh alone must
    red with three SC2034s. It did (BOOT_DOCTOR_JSON, OS_INFLIGHT, OS_STATE_DIR, rc 1).
  • lint-file-budget, lint-topology, lint-operator-strings, lint-docs-voice, lint-js all
    rc 0, each rc captured separately rather than read off a filtered tail.

Mutation battery — every class on test-lifecycle.sh

It is the last stanza before the verdict, so nothing can cascade off the mutation and the run always
reaches its verdict line. Each tree is a full git clonenot git archive, which has no
.git and carries a 6-failure floor that reads exactly like cascade damage. Every mutation was
verified applied, distinct, and correctly positioned before running, and tests/ in the clones is
byte-identical to the pushed tip, so every row exercises the shipped code.

class           expected             rc   verdict                  source status   named kill
delete          RED                  1    2874 passed, 1 failed    1               ✗ domain file test-lifecycle.sh contributed no assertions (#1400)
syntax-top      RED                  1    2874 passed, 1 failed    2               ✗ (same, exactly one)
return-top      RED                  1    2874 passed, 1 failed    0               ✗ (same, exactly one)
syntax-bottom   SURVIVE (the limit)  0    2922 passed, 0 failed    n/a             (none -- survived, by design)
none (control)  GREEN, no #1400      0    2922 passed, 0 failed    n/a             (none -- zero #1400 lines)

return-top is the decisive row. Three REDs look interchangeable; they are not. It is the only
one where source returns 0, which is exactly what the declined || domain_fail form reads as a
healthy file. That row is what makes the design choice defensible rather than merely stated.

syntax-bottom is the survivor, and it is demonstrated rather than asserted. Its verdict is
identical to the control's — 2922 passed, 0 failed, rc 0, zero #1400 lines — so on the verdict
alone the row would prove nothing. What makes it evidence is that the mutation provably fired: the
log carries test-lifecycle.sh: line 369: syntax error: unexpected end of file, and the same grep
against the none control is empty. So the suite went green with a genuinely broken domain file,
which is exactly the limit this guard has.

The syntax-top / syntax-bottom pair isolates position as the only variable: byte-identical
injected text, same syntax error at the same reported line, opposite outcomes — RED with all 48
assertions lost when it precedes them, GREEN with all 48 intact when it follows them.

Each mutation produced exactly one failure, and it is the assertion the mutation was aimed at —
so all three parts of the standard are met rather than asserted: the mutation applied, something
died, and the right thing died. No cascade, no bystander kill. delete also reconciles independently:
2922 - 48 = 2874, where 48 is the assertion delta the instrumented run measured for that domain. Two
instruments agree.

Limits, stated rather than implied

  • This proves a domain contributed at least one assertion, not that it ran to completion. A file
    that dies half way through still passes — that is row P above, and the syntax-bottom mutation is
    in the battery to demonstrate the survivor rather than assert it. Covering it needs a per-domain
    expected count, which collides with the nondeterminism in Two suite verdict lines are nondeterministic, weakening the split-proof multiset check #1325.
  • The diagnostic's hint reads "zero means a top-level return". Zero also covers a file that loaded
    cleanly and simply contained no assertions. No such file exists today (deltas are 9 to 207), and
    the RED names the file either way, so the hint is imprecise rather than wrong — flagged here rather
    than silently churning a verified tree.

Over-engineering pass

Run by hand on merit. The repo's gate keys its sentinel to the pane's branch rather than the PR's and
clears on a second invocation regardless, so its silence is not a record of anything — this is what
was actually looked at, findings either way.

  • One real finding, disclosed rather than fixed: st="${3:-0}" in domain_ran has a default no
    caller uses.
    All 48 call sites (24 stanzas x 2 branches) pass three arguments; there is no
    two-argument call anywhere. So the default is unreachable today, and if a future stanza were
    written with two arguments it would silently report source returned 0, whose hint reads "a
    top-level return"
    — a confident wrong diagnosis. Plain "$3" would abort under set -u instead,
    which is the better failure. I did not change it: domain_ran is the code the mutation battery
    above exercises, and editing it would break the byte-identity between the tested clones and the
    shipped tree for the sake of a dead default. Reviewer's call — say the word and I will change it
    and re-run the battery.
  • 18 comment lines to 8 code lines is deliberate, and each block is load-bearing, not narration:
    why a positive control rather than source's status (stops a future "simplification" back to the
    form measured worse above), why return 0 must stay (removing it double-counts the failure), and
    the limit demonstrated by syntax-bottom.
  • The duplicated domain_ran call in both branches is forced, not chosen. The single-call form
    needs a ; chain or a brace group, and shfmt splits both — see line-neutrality above.

Not run

make test's dashboard halves (pytest, node). This change touches tests/stack/ plus one line of
docs/dev/repo-map.md; the stack suite is the after-proof above. CI is the check for the rest.

The branch is 3 commits behind develop-v2. Those commits touch only dashboard/ and
os/rootfs/Dockerfile — no tests/stack/ file and not docs/dev/file-budget.tsv — so the
after-proof still describes this code and the 3841 ceiling has not moved on the base.

…ns (#1400)

run.sh runs under `set -uo pipefail` with no `-e` and computes its verdict from PASS/FAIL
alone, so a domain file that fails to source contributes nothing to either counter: the
suite prints "0 passed, 0 failed" for that whole domain, exits 0, and nothing goes red.
25 stanzas were unchecked. The sharpest case is the last one, test-lifecycle.sh, which
sits immediately above the verdict.

Each of the 24 domain stanzas now captures the counters, sources, and calls domain_ran
either way:

    _d0=$((PASS + FAIL)) && source "$HERE/x.sh" && domain_ran x.sh "$_d0" "$?" || domain_ran x.sh "$_d0" "$?"

The guard is a POSITIVE control -- did this file move the counters -- not a check of
`source`'s exit status, which answers a different question: it reads 0 for a top-level
`return` (the failure mode that leaves no diagnostic at all) and non-zero for a healthy
file that merely ended on a cleanup that failed. The status is carried into the message
only, never into the verdict.

lib.sh's own stanza stays bare on purpose. If lib.sh fails to source, PASS is unbound and
the first guarded stanza's arithmetic aborts the shell under `set -u` -- that case already
fails loud, not green.

Line-neutral in both files it touches. run.sh is 3841 against a 3841 ceiling, so it has to
be: 24 directive comments gain `disable=SC2015` in place and 24 source lines are replaced
one-for-one. `a; b; c` and brace groups were measured and rejected -- `shfmt -i 4 -d`,
which is half of `make lint`, rewrites both into three lines each and would take run.sh to
3889. shfmt preserves `&&`/`||` chains. lib.sh goes 232 -> 259, still under TARGET_LINES so
it stays unrowed.

SC2015 warns that C may run when A is true. It cannot see that domain_ran ends in an
explicit `return 0`, which is why that line is load-bearing rather than decoration: a
non-zero return would run the second branch too and count the same failure twice. The
suppression rides the directive comment that was already there, so it costs no lines and is
scoped to exactly these 24 stanzas.

Limit, stated rather than implied: this proves a domain contributed at least one assertion,
not that it ran to completion. A file that dies half way through still passes. Covering
that needs a per-domain expected count, which collides with the nondeterminism in #1325.
@VijitSingh97

Copy link
Copy Markdown
Collaborator Author

CI is RED on Shell tests, the failure is real and mine — and it exposed a second defect that is not mine and is worth more.

What broke

tests/inventory.sh:139 detects a sourced domain file with a line-anchored pattern:

grep -oE '^[[:space:]]*source "\$HERE/[A-Za-z0-9_.-]+\.sh"' tests/stack/run.sh

This PR moves source off the start of the line (_d0=$((PASS + FAIL)) && source "$HERE/x.sh" && ...), so the anchor no longer matches. Measured at this head: the pattern matches 0 of 24 domain files. The whole SOURCED set is now the single entry lib.sh, whose stanza stays bare by design.

The fix belongs in tests/inventory.sh — relax the anchor to admit the stanza prefix — and is owed re-proof (the after-proof plus the drift check itself) before this merges. The mutation battery above is unaffected: it exercises run.sh's runtime guard, not this static check.

The second defect, which is pre-existing

tests/inventory.sh:141 carries an emptiness guard whose own comment states its purpose: "the grep itself going quiet must not read as 'all present'". It did not fire. lib.sh's deliberately-bare stanza kept SOURCED non-empty at exactly one entry, so a pattern that had stopped matching 24 of 25 stanzas sailed past the guard designed to catch precisely that.

A guard whose fallback condition is "the set is empty" is defeated by one surviving member. The check needed here is a floor, not a non-empty test — the sourced count and the domain-file count are both knowable, and they should be required to agree.

Third, smaller: the drift loop reports the first offender and exits, so the CI message named one file (test-appliance-install.sh, alphabetically first) for what is actually a 24-file breakage. A reader would size the problem wrongly from the message alone.

Why this is worth stating plainly

This PR adds a guard against a domain file being silently skipped. It broke the existing guard against a domain file being silently skipped — and that guard's own anti-false-green fallback failed open. Both halves are the shape this repo keeps filing, and I would rather record that than quietly patch a regex.

The second and third findings are tests/inventory.sh defects independent of this PR and will be filed separately; #1357's inventory half is already open on the sibling counters in that file.

The domain-file drift check found sourced files with a pattern anchored to
`source` opening the line. The source-stanza guard put a prefix ahead of
every one of them, so the pattern matched 1 of 25 and the whole `SOURCED`
set collapsed to `lib.sh`, whose stanza is bare by design. CI then reported
the other 24 files as on disk but sourced by nothing.

The anchor exists to stop a commented-out `# source "$HERE/x.sh"` reading as
a live one, so it is relaxed rather than removed: nothing before the `source`
may be a `#`. A commented-out stanza is still rejected, indented or not, and
so is a `source` sitting after a trailing comment on a live line.

Measured at the head, not reasoned: 25 of 25 stanzas match, no duplicates,
and the three commented shapes are all still refused. `bash tests/inventory.sh`
exits 0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant