From 2b3772664aa9aef488b349615218e9c0c07b9fa7 Mon Sep 17 00:00:00 2001 From: Oleksandr Shyshatskyi Date: Mon, 24 Aug 2026 19:37:18 +0300 Subject: [PATCH] Close six gates that passed while checking nothing Every one of these reported success on a condition it could not fail, which is worse than having no gate at all -- this repo has been bitten by it three times already (test-abi.sh, lint-danger, the old skip protocol) and the harness docstrings cite those as the reason it exists. run_pytest scored a run that collected NOTHING as a pass. pytest exits 5 and writes tests="0"; passed = total - failures - skipped then gave 0/0/0 and the gate went green, with proc.returncode never read. Renaming test/integration/, breaking a conftest import or adding a collect_ignore would have stopped all 54 tests while make test and both CI jobs stayed green. Reproduced before fixing. check_no_bashisms passed when shellcheck examined no files: with no targets it writes usage to stderr and exits 1, leaving the SC3xxx filter empty. check_shell_syntax has an empty-target guard; this one did not. Moving payload/ would have retired the ash-compatibility check in silence. Its exit code is now checked too, so a usage or file error cannot read as "no findings". case-install.sh's "a service owns Klipper startup" grepped for the string start.sh anywhere under init.d/ -- and S70klipper and S60web both name it in their header COMMENTS, so deleting the line that actually runs it still passed. Now requires a non-comment line. case-install.sh scored the genuine FlashForge binary at firmwareExe as ok ("no UI replacement requested"). patch.sh installs the wrapper unconditionally and HelixScreen is the only UI, so that outcome means the install did not take -- and it silently switched off four further assertions plus the whole boot-3 UI block, making "the install produced nothing" a passing run. case-mcu-bringup.sh printed SKIP and exit 0 when the interpreter was not executable, which Replica.run_case scores as a passing gate: the import test, the resolution check, the bring-up run, the ttyS7 coverage and the LD_LIBRARY_PATH control were all skipped and reported clean. seed-prog.sh already hard-fails on a MISSING interpreter, so this window is a broken replica, not an absent feature. It fails now. verify.sh's ship-boundary check uses relative finds, so from any cwd but the repo root host.md5 is empty, the join matches nothing, and it reported "package carries nothing from bin/, test/ or docker/" having compared nothing -- the only check keeping config.env off a USB stick. Full suite with every gate tightened: 12 passed, 0 failed, 0 skipped. Nothing was hiding behind them. Co-Authored-By: Claude Opus 5 (1M context) --- bin/verify.sh | 9 ++++++++- test/integration/printer/case-install.sh | 15 +++++++++++++-- test/integration/printer/case-mcu-bringup.sh | 12 ++++++++++-- test/run-tests.py | 20 ++++++++++++++++++++ 4 files changed, 51 insertions(+), 5 deletions(-) diff --git a/bin/verify.sh b/bin/verify.sh index 69e6221..0d7661c 100755 --- a/bin/verify.sh +++ b/bin/verify.sh @@ -189,7 +189,14 @@ find bin test docker -type f 2>/dev/null | xargs -r md5sum 2>/dev/null \ find "$T/ship" -type f 2>/dev/null | xargs -r md5sum 2>/dev/null > "$T/ship.md5" LEAK=$(awk 'NR==FNR{h[$1];next} $1 in h {print}' "$T/host.md5" "$T/ship.md5" \ | sed "s|$T/ship/||") -if [ -z "$LEAK" ]; then +# Those finds are RELATIVE, so from any cwd but the repo root host.md5 comes +# out empty, the join matches nothing and this reports ok having compared +# nothing -- retiring the only check that keeps config.env and the host tooling +# off a USB stick. common.sh cds to the root, so this is a guard against a +# future caller that does not. +if [ ! -s "$T/host.md5" ]; then + bad "ship-boundary check compared nothing (no files found under bin/, test/, docker/ -- wrong cwd?)" +elif [ -z "$LEAK" ]; then ok "package carries nothing from bin/, test/ or docker/" else bad "host-side files leaked into the package:" diff --git a/test/integration/printer/case-install.sh b/test/integration/printer/case-install.sh index 35146e5..6a4544f 100755 --- a/test/integration/printer/case-install.sh +++ b/test/integration/printer/case-install.sh @@ -135,7 +135,14 @@ if head -c 2 "$FE" 2>/dev/null | grep -q '#!'; then && ok "the wrapper launches HelixScreen" \ || bad "BRICK: wrapper installed but it starts no UI at all" elif head -c 4 "$FE" | grep -q 'ELF'; then - ok "stock firmwareExe binary in place (no UI replacement requested)" + # bin/patch.sh installs the wrapper unconditionally and HelixScreen is the + # only UI, so the genuine binary sitting here means the install did not + # take. This used to print ok, which also switched off the wrapper parse, + # the HelixScreen check, the Klipper-service check and the whole boot-3 UI + # block -- making "the install produced nothing" a passing run. + bad "BRICK: stock firmwareExe binary in place -- the wrapper was not installed" +else + bad "BRICK: firmwareExe is neither the wrapper nor the stock binary" fi # Every shell script the mod puts on the machine has to parse with THIS shell. @@ -150,7 +157,11 @@ done [ "$BADPARSE" = 0 ] && ok "every installed script parses under the printer's busybox ash" if [ -d /usr/data/anvil/init.d ]; then - grep -rq 'start\.sh' /usr/data/anvil/init.d/ 2>/dev/null \ + # Must be an actual invocation, not a mention. S70klipper and S60web both + # name start.sh in their header comments, so a bare grep for the string + # passed even with the line that runs it deleted -- the lint-danger + # failure mode this suite exists to avoid. + grep -rq '^[^#]*start\.sh' /usr/data/anvil/init.d/ 2>/dev/null \ && ok "a service owns Klipper startup" \ || bad "nothing starts Klipper -- UI would boot with no motion or heaters" elif [ "$WRAPPER" = 1 ]; then diff --git a/test/integration/printer/case-mcu-bringup.sh b/test/integration/printer/case-mcu-bringup.sh index 4e718e3..6bad57b 100755 --- a/test/integration/printer/case-mcu-bringup.sh +++ b/test/integration/printer/case-mcu-bringup.sh @@ -31,8 +31,16 @@ fi ok "payload ships bin/ff-mcu-bringup.py" if [ ! -x "$PY" ]; then - skip "no $PY in this replica -- set PROG_DUMP to a factory image" - exit 0 + # `exit 0` here reported a PASSING gate: Replica.run_case only reads the + # exit code, so everything below -- the import test, the resolution check, + # the actual bring-up run, the ttyS7 coverage and the LD_LIBRARY_PATH + # negative control -- was skipped and scored as clean. That is the + # "SKIP printed, exit 0, counted as ok" protocol ffsim was written to end. + # seed-prog.sh already hard-fails when the interpreter is MISSING, so + # reaching here means it is present but not executable, which is a broken + # replica rather than an absent feature. + bad "$PY is not executable -- the replica has no usable interpreter" + exit 1 fi ok "interpreter present at $PY" diff --git a/test/run-tests.py b/test/run-tests.py index 33595c6..e10b39c 100755 --- a/test/run-tests.py +++ b/test/run-tests.py @@ -90,10 +90,21 @@ def check_no_bashisms(r): + sorted(ROOT.glob("payload/init.d/S*")) + [ROOT / "payload" / "firmwareExe"]) targets = [str(t) for t in targets if t.is_file()] + # With no targets shellcheck writes usage to stderr and exits 1, leaving + # `hits` empty -- a green gate that examined nothing. check_shell_syntax + # guards this; this one did not, so moving payload/ would have retired the + # ash-compatibility check silently. + if not targets: + raise Fail("no payload scripts found to check -- has payload/ moved?") proc = subprocess.run(["shellcheck", "-s", "dash", "-f", "gcc"] + targets, capture_output=True, text=True) hits = [ln for ln in (proc.stdout or "").split("\n") if "SC3" in ln or "SC2039" in ln] + # shellcheck exits 1 for findings (which `hits` covers) but also for a + # usage or file error, which would otherwise pass unnoticed. + if not hits and proc.returncode not in (0, 1): + raise Fail("shellcheck failed to run (exit %d): %s" + % (proc.returncode, (proc.stderr or "").strip()[:200])) if hits: for ln in hits[:10]: r._say(" %s\n" % ln) @@ -137,6 +148,15 @@ def run_pytest(r): or "no reason given")) passed = total - failures - skipped + # A run that collected nothing is not a pass. pytest exits 5 and writes + # tests="0", which used to give passed=0/failures=0/skipped=0 and a green + # gate -- so renaming test/integration/, breaking a conftest import or + # adding a collect_ignore would silently stop the whole Python lane while + # make test and CI stayed green. That is the exact failure this harness + # was written to end (see test-abi.sh in docs/testing.md). + if not total: + raise Fail("pytest collected no tests (exit %d) -- collection is " + "broken, or the suite has moved" % proc.returncode) if failures: r._say((proc.stdout or "")[-4000:]) raise Fail("pytest: %d failed, %d passed, %d skipped"