A negated command only asserts in final position, and one that could never pass #16

Merged

marcos-mendez merged 2 commits from test/negations-that-assert into main

2026-09-29 00:59:10 -03:00

marcos-mendez commented

2026-09-28 05:29:28 -03:00

(Migrated from github.com)

Every negation in these suites now asserts wherever it stands. Six did not assert at all — and turning one on found a second bug in the same test.

Closes #15. Part of Keel-Linux/tracker#19, which has the organization-wide sweep.

Why a bare `!` is inert

A bats test body has no assertions of its own: its verdict is the exit status of the last command it ran. Bash does not apply errexit to a negated command, so ! cmd decides the test in final position and is inert everywhere else. shellcheck grades the two apart, error: for the inert ones and note: for the rest:

shellcheck -f gcc --shell=bash tests/*.bats | grep SC2314

What was not being checked

All six are about trusting a proxy.

Where	Was not asserting
nginx.bats 19, 29	that the shipped `conf.d` default trusts nobody, and that it never matches `geo` on `$remote_addr`. Two of those four assertions ran.
nodebb.bats 117	`nodebb_valid_proxy "2001:db8::13; }"` — the value that would close the `geo` block and append directives of its own. The injection case. Only `hello` decided anything.
nodebb.bats 135, 136	that the rendered `geo` is on `$realip_remote_addr` and never on `$remote_addr` — the redirect loop the file's own comment warns about at length.
nodebb.bats 177, 178	that no address means no trust.

All of them are run ! now, together with the eight that were in final position and did assert: a line whose meaning depends on its position is the trap itself.

The bug this found

nodebb.bats, "proxy_conf without an address trusts nobody", asserted:

! grep -q set_real_ip_from <<< "$output"

Unanchored — against a file whose own explanatory comment names the directive:

$ nodebb_proxy_conf '' | grep -n set_real_ip_from
6:# that is not a detail. This same file sets set_real_ip_from with

So that assertion could never have held, for any input, and nobody could know, because nothing ever ran it. It is anchored now:

run ! grep -q '^set_real_ip_from' <<< "$conf"

which is the property that was meant, and the spelling the positive case at line 167 already used (^set_real_ip_from 2001:db8::13;$). The rendered file is correct — with no address it emits no set_real_ip_from directive. Only the assertion was wrong, so nothing shipped broken.

A second shape of the same defect

run replaces $output, $status and $lines. Two tests here do run nodebb_proxy_conf … and then grep <<< "$output" several times; once the first of those becomes a run !, the next one greps the refutation's empty output and passes for a new reason. Both tests now keep the rendered file in a local first:

run nodebb_proxy_conf ""
[ "$status" -eq 0 ]
# every `run` below replaces $output, so keep the rendered file first
local conf=$output

That was caught by an audit over every converted site, not by the suite — the vacuous pass is green by construction. It is written up in the docs/traps.md entry that goes with this sweep.

What else was verified

The shipped overlay/etc/nginx/conf.d/nodebb-proxy.conf and the rendered output were both read directly, and nodebb_valid_proxy was run against all five of its cases by hand, before the conversion. Everything except the assertion above is correct.

Test plan

The validator's five cases and both rendered files verified by hand, before the conversion.
shellcheck -f gcc --shell=bash --severity=error --include=SC2314 tests/*.bats reports nothing.
Every converted site audited for run clobbering $output/$status/$lines; the two real cases fixed.
bats tests/*.bats green, 134 tests.

tests/coverage.sh, threshold 95, unchanged:

100.00  31/31    zz-project-packages
100.00  43/43    nodebb.sh
 96.97  32/33    40nodebb
100.00  137/137  boot-test-lib.sh
100.00  54/54    keel-archive-check

Tests ship nothing, so package / changelog passes by reporting that nothing that ships changed. No changelog entry and no version bump.

Every negation in these suites now asserts wherever it stands. Six did not assert at all — and turning one on found a second bug in the same test. Closes #15. Part of Keel-Linux/tracker#19, which has the organization-wide sweep. ## Why a bare `!` is inert A bats test body has no assertions of its own: its verdict is the exit status of the last command it ran. Bash does not apply errexit to a negated command, so `! cmd` decides the test in final position and is inert everywhere else. shellcheck grades the two apart, `error:` for the inert ones and `note:` for the rest: shellcheck -f gcc --shell=bash tests/*.bats | grep SC2314 ## What was not being checked All six are about trusting a proxy. | Where | Was not asserting | |---|---| | nginx.bats 19, 29 | that the shipped `conf.d` default trusts nobody, and that it never matches `geo` on `$remote_addr`. Two of those four assertions ran. | | nodebb.bats 117 | `nodebb_valid_proxy "2001:db8::13; }"` — the value that would close the `geo` block and append directives of its own. The injection case. Only `hello` decided anything. | | nodebb.bats 135, 136 | that the rendered `geo` is on `$realip_remote_addr` and never on `$remote_addr` — the redirect loop the file's own comment warns about at length. | | nodebb.bats 177, 178 | that no address means no trust. | All of them are `run !` now, together with the eight that were in final position and did assert: a line whose meaning depends on its position is the trap itself. ## The bug this found `nodebb.bats`, "proxy_conf without an address trusts nobody", asserted: ! grep -q set_real_ip_from <<< "$output" Unanchored — against a file whose own explanatory comment names the directive: $ nodebb_proxy_conf '' | grep -n set_real_ip_from 6:# that is not a detail. This same file sets set_real_ip_from with So **that assertion could never have held, for any input**, and nobody could know, because nothing ever ran it. It is anchored now: run ! grep -q '^set_real_ip_from' <<< "$conf" which is the property that was meant, and the spelling the positive case at line 167 already used (`^set_real_ip_from 2001:db8::13;$`). The rendered file is correct — with no address it emits no `set_real_ip_from` directive. Only the assertion was wrong, so nothing shipped broken. ## A second shape of the same defect `run` replaces `$output`, `$status` and `$lines`. Two tests here do `run nodebb_proxy_conf …` and then grep `<<< "$output"` several times; once the first of those becomes a `run !`, the next one greps the refutation's empty output and passes for a new reason. Both tests now keep the rendered file in a local first: run nodebb_proxy_conf "" [ "$status" -eq 0 ] # every `run` below replaces $output, so keep the rendered file first local conf=$output That was caught by an audit over every converted site, not by the suite — the vacuous pass is green by construction. It is written up in the `docs/traps.md` entry that goes with this sweep. ## What else was verified The shipped `overlay/etc/nginx/conf.d/nodebb-proxy.conf` and the rendered output were both read directly, and `nodebb_valid_proxy` was run against all five of its cases by hand, before the conversion. Everything except the assertion above is correct. ## Test plan - [x] The validator's five cases and both rendered files verified by hand, before the conversion. - [x] `shellcheck -f gcc --shell=bash --severity=error --include=SC2314 tests/*.bats` reports nothing. - [x] Every converted site audited for `run` clobbering `$output`/`$status`/`$lines`; the two real cases fixed. - [x] `bats tests/*.bats` green, 134 tests. - [x] `tests/coverage.sh`, threshold 95, unchanged: 100.00 31/31 zz-project-packages 100.00 43/43 nodebb.sh 96.97 32/33 40nodebb 100.00 137/137 boot-test-lib.sh 100.00 54/54 keel-archive-check - [x] Tests ship nothing, so `package / changelog` passes by reporting that nothing that ships changed. No changelog entry and no version bump.

marcos-mendez commented

2026-09-29 00:54:34 -03:00

(Migrated from github.com)

Reviewed: every converted refutation goes red when its property is broken; emitting set_real_ip_from with no trusted address fails the anchored check. None reads the output of the run it replaced. No changes needed; merged main so the renamed appliance check runs on this head.