From 170f8434818b9b2eba7b7dafdfacdab1cc281333 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sat, 22 Aug 2026 12:16:18 +0200 Subject: [PATCH] Make the jailed lib/tests gate able to fail, without breaking measurement The jail recorded each case's exit code but reported only timeouts, discarding every other non-zero status. A suite with a failing assertion exited 0 through the push gate -- the same blind spot that let an earlier session ship 2 failing assertions in the nc_php suite. Found by negative control, not by reading: a deliberately failing assertion still exited 0. Propagating that status unconditionally was wrong, and caught before landing: the jail's own usage example passes a `bogus` case that exits non-zero to cover an error path, so a clean 5/5 = 100.00% run started failing. That is the mode the 105-lib campaign runs in. So the behaviour sits behind --fail-on-case-error, default off, passed only by ci_mirror.sh and shell-tests.yml. Verified in all four quadrants: measurement + error case = 0, gate + error case = 1, gate + clean suite = 0, gate + broken assertion = 1. Both gates also pass --min 0: with no --measure the jail was measuring run_all.sh itself against its default --min 100 and failing the push at 54.55%, a coverage verdict neither gate asked for. ci_mirror.sh now prints the jail's output on failure, so a red gate can be diagnosed. The added flag pushed the jail over the 250-line cap, so the in-namespace case runner moved to lib/shell_coverage_case_runner.sh, following the existing lib/shell_coverage_jail_setup.sh split. All four quadrants were re-run after the extraction. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_016sxxsDHWgdRhmdmzNMA6XM --- .github/workflows/shell-tests.yml | 6 +- .../jailed-libtests-gate-2026-08-22.json | 21 ++++++ ...il-propagates-case-failure-2026-08-22.json | 70 ++++++++++++++++++ meta/scripts/ci_mirror.sh | 15 +++- .../scripts/lib/shell_coverage_case_runner.sh | 64 +++++++++++++++++ meta/scripts/shell_coverage_jail.sh | 71 +++++++++---------- 6 files changed, 205 insertions(+), 42 deletions(-) create mode 100644 docs/superpowers/contracts/jailed-libtests-gate-2026-08-22.json create mode 100644 docs/superpowers/evidence/jail-propagates-case-failure-2026-08-22.json create mode 100755 meta/scripts/lib/shell_coverage_case_runner.sh diff --git a/.github/workflows/shell-tests.yml b/.github/workflows/shell-tests.yml index 7b2ee9b8..146d7ad9 100644 --- a/.github/workflows/shell-tests.yml +++ b/.github/workflows/shell-tests.yml @@ -83,8 +83,12 @@ jobs: failed=$((failed + 1)) continue fi + # --min 0: this step gates on the ASSERTIONS, not on coverage. + # The jail's default --min 100 would otherwise measure the + # runner itself and fail on a number this step never asked for; + # the coverage bar belongs to check_shell_coverage.sh. cmd=(bash meta/scripts/shell_coverage_jail.sh \ - --subject "$r" "${jail_args[@]}" -- "") + --subject "$r" "${jail_args[@]}" --min 0 --fail-on-case-error -- "") else cmd=("$r") fi diff --git a/docs/superpowers/contracts/jailed-libtests-gate-2026-08-22.json b/docs/superpowers/contracts/jailed-libtests-gate-2026-08-22.json new file mode 100644 index 00000000..59372512 --- /dev/null +++ b/docs/superpowers/contracts/jailed-libtests-gate-2026-08-22.json @@ -0,0 +1,21 @@ +{ + "title": "Make the lib/tests gate run jail-requiring suites, and make it able to fail", + "objective": "Unblock the push gate and CI for suites that must execute inside shell_coverage_jail.sh, and close the fail-open hole that let a suite with a failing assertion exit 0 through that gate -- without changing the jail's behaviour in the measurement mode the 105-lib shell-coverage campaign depends on.", + "acceptance_criteria": [ + "A suite declares its jail requirement as data (a jail_args file beside run_all.sh); neither ci_mirror.sh nor shell-tests.yml names any suite path, so a future jail-requiring suite needs no edit to either runner.", + "Both runners fail closed on a jail_args file that is present but empty: reported as a configuration error, never silently skipped.", + "Gate mode fails on a failing assertion: the features/lib suite with a deliberately broken _t_eq injected before _t_report exits non-zero.", + "Gate mode does not false-positive: the unmodified features/lib suite exits 0.", + "Measurement mode is unchanged: a subject with an error-path case that exits non-zero still reports its coverage and exits 0, so --fail-on-case-error is off by default.", + "The extracted jail_args flags reproduce the independently measured dot_resolver_install.sh 39/39 = 100.00%.", + "The five lib/tests suites that do not require the jail still run bare and pass.", + "Every touched shell file is within the 250-line cap and shellcheck-clean, with no suppressions added." + ], + "out_of_scope": [ + "The REFUSING guards in the individual test files: they are what stop a test run from writing the developer's real /etc, and are deliberately untouched.", + "Wiring check_shell_coverage.sh into any hook -- that arms only when the allowlist reaches zero.", + "The kcov mis-measurement of rpi_nc_install.sh, which is tracked separately as an instrument defect.", + "The unrelated python_pkg/token_audit/ files a concurrent session staged into the shared index." + ], + "verifier": "Four quadrants via meta/scripts/shell_coverage_jail.sh: measurement+error-case = 0 with coverage printed; gate+error-case = 1; gate+clean features/lib suite = 0; gate+injected-failing-assertion = 1. Plus bash meta/scripts/check_file_length.sh --all showing no violation, shellcheck on the three shell files, and a successful git push through the ci-mirror pre-push hook." +} diff --git a/docs/superpowers/evidence/jail-propagates-case-failure-2026-08-22.json b/docs/superpowers/evidence/jail-propagates-case-failure-2026-08-22.json new file mode 100644 index 00000000..fb8d1302 --- /dev/null +++ b/docs/superpowers/evidence/jail-propagates-case-failure-2026-08-22.json @@ -0,0 +1,70 @@ +{ + "intent": "Make the jailed lib/tests gate actually able to fail. The jail recorded each case's exit code but reported only timeouts (124/137), discarding every other non-zero status, so a test suite with a failing assertion exited 0 through the push gate. A gate that cannot fail reports safe while checking nothing.", + "scope": [ + "meta/scripts/shell_coverage_jail.sh: new --fail-on-case-error flag (default OFF); record non-zero case exits and fail only when the flag is set", + "meta/scripts/ci_mirror.sh: pass --min 0 to the suite-running invocation", + ".github/workflows/shell-tests.yml: same --min 0", + "Non-goal: the coverage bar stays check_shell_coverage.sh's job, per-lib and --measure'd; this gate asks only 'did the assertions pass?'", + "meta/scripts/lib/shell_coverage_case_runner.sh: new; the in-namespace case runner extracted to hold shell_coverage_jail.sh under the 250-line cap" + ], + "changes": [ + "Found by negative control, not by reading: a deliberately failing assertion injected into test_dot_resolver_install.sh still exited 0. This is the same blind spot that let an earlier session ship 2 failing assertions in the nc_php suite.", + "The jail was built as a MEASUREMENT instrument, where a subject exiting non-zero is normal (an installer given a bogus arg still yields coverage). Using it as a GATE requires propagating that status, so the non-zero exit is now recorded rather than swallowed.", + "Coverage is still reported before the failure check: the number is wanted even when a case failed, and suppressing it would hide the diagnosis behind the symptom.", + "--min 0 on both gate invocations: with no --measure, the jail measured run_all.sh itself against its default --min 100 and failed the push at 54.55% -- a number neither gate asked for.", + "Caught in review before landing: propagating the status UNCONDITIONALLY broke measurement mode. The jail's own usage example passes a `bogus` case that exits non-zero to cover an error path; measured on a minimal subject, a clean 5/5 = 100.00% run started exiting 1. The behaviour is now behind --fail-on-case-error, passed only by the two gates. Measurement mode -- what the 105-lib campaign runs -- is unchanged.", + "ci_mirror.sh captures the jail's output and prints it on the failure path only. Discarding it left a red gate reporting 'suite failed: ' with no warn: or FAIL: lines to diagnose from.", + "The added flag pushed shell_coverage_jail.sh to 285 lines and the file-length-cap hook rejected the commit. The in-namespace case runner (a self-contained heredoc block) was extracted to lib/shell_coverage_case_runner.sh, following the existing lib/shell_coverage_jail_setup.sh split. Jail 240 lines, new lib 64. All four verification quadrants were re-run after the extraction to confirm the refactor changed no behaviour." + ], + "verification": [ + { + "command": "shell_coverage_jail.sh --subject errsubj.sh --min 100 -- \"\" bogus (measurement mode, error-path case)", + "result": "pass", + "evidence": "exit=0, errsubj.sh: 5/5 lines = 100.00%. Confirms the regression is gone: before the flag this exited 1 on a clean 100%." + }, + { + "command": "shell_coverage_jail.sh --subject errsubj.sh --min 100 --fail-on-case-error -- \"\" bogus (gate mode)", + "result": "pass", + "evidence": "exit=1, 'Error: 1 case(s) exited non-zero'. Same subject, opposite verdict -- the flag is what separates the modes." + }, + { + "command": "jailed features/lib run_all.sh --min 0 --fail-on-case-error, unmodified suite", + "result": "pass", + "evidence": "exit=0. No false positive; a green suite passes the gate. This is also the first time the 'assertions pass' claim has been machine-checked -- the gate could not fail before." + }, + { + "command": "same, with `_t_eq \"a\" \"b\"` injected BEFORE _t_report, then restored", + "result": "pass", + "evidence": "exit=1. The negative control that proves the gate can fail. git diff --stat empty afterwards, confirming the injection was fully reverted." + }, + { + "command": "shellcheck meta/scripts/shell_coverage_jail.sh meta/scripts/ci_mirror.sh", + "result": "pass", + "evidence": "no findings; no suppressions added." + }, + { + "command": "git diff --cached --name-only", + "result": "pass", + "evidence": "only my files. A concurrent session had staged 8 python_pkg/token_audit/ files into the shared index; unstaged with `git restore --staged`, left intact on disk." + }, + { + "command": "bash meta/scripts/check_file_length.sh --all", + "result": "pass", + "evidence": "all checked files are within 250 lines (was: 285 for shell_coverage_jail.sh, +35 over)." + }, + { + "command": "all four quadrants re-run AFTER the lib extraction", + "result": "pass", + "evidence": "measurement+error=0, gate+error=1, gate+clean=0, gate+broken=1 -- identical to pre-refactor. shellcheck clean on all three shell files." + } + ], + "risks": [ + "The two modes now differ by a flag. A future gate that forgets --fail-on-case-error is silently fail-open again -- the exact defect this commit fixes. The negative control in this evidence is the check that catches it; re-run it if either gate is edited.", + "CI needs kcov on ubuntu-latest (apt-installable, added to the existing install step); an apt outage turns shell-tests red for an infrastructure reason.", + "jail_args is positional shell words, one per line; a flag and its value on one line would be passed as a single argument and rejected by the jail." + ], + "rollback": [ + "git revert the commit; the jail returns to swallowing non-zero case exits and both gates return to their default --min 100.", + "After rollback, re-run the negative control: it should exit 0 again, confirming the fail-open behaviour is back and the revert was complete." + ] +} diff --git a/meta/scripts/ci_mirror.sh b/meta/scripts/ci_mirror.sh index 4703a200..f5d9415c 100755 --- a/meta/scripts/ci_mirror.sh +++ b/meta/scripts/ci_mirror.sh @@ -146,9 +146,20 @@ run_jailed_suite() { fail "jail_args is present but empty: $args_file" fi - (cd "$CHECKOUT" && bash meta/scripts/shell_coverage_jail.sh \ - --subject "$runner" "${jail_args[@]}" -- "" >/dev/null) || + # --min 0: this gate asks "did the assertions pass?", not "is the lib at + # 100%?". Without it the jail measures the runner itself against its + # default --min 100 and fails the push on a number no one asked for. + # The coverage bar is check_shell_coverage.sh's job, per-lib and --measure'd. + # Output is captured, not discarded: on success it is noise, but on failure + # it holds the `warn: case exited` line and the suite's own FAIL: lines. + # Without this the gate reports only "suite failed" and cannot be diagnosed. + local out + if ! out="$(cd "$CHECKOUT" && bash meta/scripts/shell_coverage_jail.sh \ + --subject "$runner" "${jail_args[@]}" \ + --min 0 --fail-on-case-error -- "" 2>&1)"; then + printf '%s\n' "$out" >&2 fail "jailed lib/tests suite failed: $runner" + fi } run_shell_test_gate() { diff --git a/meta/scripts/lib/shell_coverage_case_runner.sh b/meta/scripts/lib/shell_coverage_case_runner.sh new file mode 100755 index 00000000..a5265ed0 --- /dev/null +++ b/meta/scripts/lib/shell_coverage_case_runner.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash + +# ============================================================================ +# write_case_runner — emit the script that executes the cases INSIDE the jail. +# +# Split out of shell_coverage_jail.sh to hold that file under the 250-line cap +# after --fail-on-case-error was added. Sourced, never run: it needs $JAIL from +# its caller. +# +# The runner is written to a file rather than inlined into the `unshare` +# command because it executes in the child namespace: a quoted inline block +# would have to defer every expansion to the child shell, which reads as a +# quoting bug and trips SC2016. +# ============================================================================ + +write_case_runner() { + # The runner below executes INSIDE the namespace, so it is written out as + # its own file rather than inlined: a quoted inline block would have to + # defer every expansion to the child shell, which reads as a quoting bug + # and trips SC2016. + cat >"$JAIL/run_cases.sh" <<'INNER' +#!/usr/bin/env bash +set -u +jail="$1" +subject_dir="$2" +subject_base="$3" +measure_base="$4" +case_timeout="$5" +. "$jail/mounts.sh" +export PATH="$jail/bin:/usr/bin:/bin" +export HOME="$jail/home" +USER="${USER:-root}" +export USER +export SUDO_USER="$USER" +cd "$subject_dir" || exit 1 +while IFS= read -r line; do + read -r -a case_args <<<"$line" + # Bounded, and with stdin closed. Both matter: a subject that reads stdin + # blocks forever on the caller's terminal (a `crontab -` fallback did + # exactly this and hung a run for the full two minutes with no output), + # and a subject that spins never yields at all. A timed-out case is + # reported rather than swallowed, because a silent timeout looks + # identical to an unreachable line in the coverage report. + timeout --kill-after=10s "$case_timeout" \ + kcov --include-pattern="$measure_base" "$jail/cov" \ + "./$subject_base" ${case_args[@]+"${case_args[@]}"} \ + >/dev/null 2>&1 &2 + printf '%s\n' "$rc" >>"$jail/case_failures" + elif [[ $rc -ne 0 ]]; then + # A non-zero case is only noise when measuring (an installer that + # exits 1 on a bogus arg still yields coverage), but it is the whole + # signal when the subject is a TEST SUITE. Record it and let the + # caller decide; swallowing it made a deliberately-broken assertion + # exit 0 through the push gate. + printf 'warn: case exited %d: %s\n' "$rc" "$line" >&2 + printf '%s\n' "$rc" >>"$jail/case_failures" + fi +done <"$jail/cases" +INNER + chmod +x "$JAIL/run_cases.sh" +} diff --git a/meta/scripts/shell_coverage_jail.sh b/meta/scripts/shell_coverage_jail.sh index b11d0a43..04255f60 100755 --- a/meta/scripts/shell_coverage_jail.sh +++ b/meta/scripts/shell_coverage_jail.sh @@ -46,6 +46,11 @@ MIN_PERCENT=100 # Per-case wall-clock ceiling. A hung case cannot be distinguished from a slow # one without a bound, and an unbounded run blocks a hook indefinitely. CASE_TIMEOUT="120s" +# Off by default: when MEASURING, a case that exits non-zero is normal and +# wanted -- the usage example's `bogus` case exists precisely to cover an +# error path. Only a GATE, whose subject is a test suite, wants that status +# to fail the run. See --fail-on-case-error. +FAIL_ON_CASE_ERROR=0 BIND_PATHS=() EXTRA_SHIMS=() CASES=() @@ -69,6 +74,12 @@ Usage: $SCRIPT_NAME --subject [options] -- [...] --seed-file create an empty file at inside the jail --timeout per-case wall-clock ceiling (default 120s) --min fail below this (default 100) + --fail-on-case-error + exit non-zero if any case exits non-zero. For GATE use, + where the subject is a test suite and a failing + assertion must fail the run. Off by default: when + measuring, an error-path case exiting non-zero is the + point of running it. -- ... one invocation each; "" means "no arguments" Example: @@ -113,6 +124,10 @@ while [[ $# -gt 0 ]]; do MIN_PERCENT="$2" shift 2 ;; + --fail-on-case-error) + FAIL_ON_CASE_ERROR=1 + shift + ;; -h | --help) usage ;; --) shift @@ -161,6 +176,10 @@ trap cleanup EXIT # shellcheck source=./lib/shell_coverage_jail_setup.sh . "$(dirname "${BASH_SOURCE[0]}")/lib/shell_coverage_jail_setup.sh" +# The in-namespace case runner, likewise split for the 250-line cap. +# shellcheck source=./lib/shell_coverage_case_runner.sh +. "$(dirname "${BASH_SOURCE[0]}")/lib/shell_coverage_case_runner.sh" + main() { validate_requirements build_jail @@ -180,44 +199,7 @@ main() { local tree_before tree_before="$(cd "$REPO_ROOT" && git status --porcelain)" - # The runner below executes INSIDE the namespace, so it is written out as - # its own file rather than inlined: a quoted inline block would have to - # defer every expansion to the child shell, which reads as a quoting bug - # and trips SC2016. - cat >"$JAIL/run_cases.sh" <<'INNER' -#!/usr/bin/env bash -set -u -jail="$1" -subject_dir="$2" -subject_base="$3" -measure_base="$4" -case_timeout="$5" -. "$jail/mounts.sh" -export PATH="$jail/bin:/usr/bin:/bin" -export HOME="$jail/home" -USER="${USER:-root}" -export USER -export SUDO_USER="$USER" -cd "$subject_dir" || exit 1 -while IFS= read -r line; do - read -r -a case_args <<<"$line" - # Bounded, and with stdin closed. Both matter: a subject that reads stdin - # blocks forever on the caller's terminal (a `crontab -` fallback did - # exactly this and hung a run for the full two minutes with no output), - # and a subject that spins never yields at all. A timed-out case is - # reported rather than swallowed, because a silent timeout looks - # identical to an unreachable line in the coverage report. - timeout --kill-after=10s "$case_timeout" \ - kcov --include-pattern="$measure_base" "$jail/cov" \ - "./$subject_base" ${case_args[@]+"${case_args[@]}"} \ - >/dev/null 2>&1 &2 - fi -done <"$jail/cases" -INNER - chmod +x "$JAIL/run_cases.sh" + write_case_runner # Seeds are created after the mounts, from inside the namespace: a path # under a bound target does not exist until that target is mounted. @@ -243,8 +225,19 @@ INNER exit 1 fi + # Report coverage first -- the number is wanted even when a case failed, + # and suppressing it would hide the diagnosis behind the symptom. + local report_rc=0 python3 "$REPO_ROOT/meta/scripts/shell_coverage_report.py" \ - "$JAIL/cov" "$measure_base" "$MIN_PERCENT" + "$JAIL/cov" "$measure_base" "$MIN_PERCENT" || report_rc=$? + + if [[ $FAIL_ON_CASE_ERROR -eq 1 && -s "$JAIL/case_failures" ]]; then + printf 'Error: %d case(s) exited non-zero; see the warn: lines above\n' \ + "$(wc -l <"$JAIL/case_failures")" >&2 + return 1 + fi + + return "$report_rc" } main "$@"