From 46c8f065e850c868dee5f5c4598fb73f528697e7 Mon Sep 17 00:00:00 2001 From: Krzysztof kuhy Rudnicki Date: Sun, 23 Aug 2026 15:51:29 +0200 Subject: [PATCH] Wire the shell ratchet into pre-commit, and test the coverage tooling check_shell_coverage.sh has existed for a while and was never in .pre-commit-config.yaml, so nothing ran it: a new shell library with no suite beside it could land freely. Wiring it was unaffordable while the gate measured a jail-run 100% bar (--all took over 600s). After 578c4a6c dropped that, --all is 0.31s and the per-file mode this hook uses is milliseconds, so it goes in. Also fixes a bug 578c4a6c introduced: the failure path still interpolated ${COVERAGE_BAR}, which that commit deleted, so every rejection printed "COVERAGE_BAR: unbound variable" and advised a 100% line-coverage bar that no longer exists. The message now describes what is enforced. Three libs blocked the wiring -- meta/scripts/lib/shell_coverage_{bar, case_runner,jail_setup}.sh, the code that BUILDS the jail, and so the one corner of the repo the jail cannot measure itself. They are pure file-writers, so they get a suite that calls them against a temp $JAIL and asserts on what was written: 48 assertions, nothing mounted, no unshare, no kcov. Several assertions check that an emitted file contains an UNEXPANDED ${BASH_SOURCE} or $1 -- the whole point of the quoted heredocs the jail relies on. Written as single-quoted literals that is SC2016 on every line, so the needles compose the dollar from an exported constant instead. No suppressions were added. check_shell_coverage.sh --all: 3 BLOCK -> 0 BLOCK hook, repo-wide: 0.16s new suite: 48 assertions, green Proved the ratchet still bites: an untested lib dropped into mtk_root/lib is refused with "needs: .../tests/run_all.sh". Probe removed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01PJrqKERAo5QUeEHWS5Asgp --- .pre-commit-config.yaml | 13 ++ ...ire-shell-coverage-ratchet-2026-08-23.json | 19 +++ ...ire-shell-coverage-ratchet-2026-08-23.json | 44 +++++++ meta/scripts/check_shell_coverage.sh | 10 +- .../lib/tests/coverage_tool_harness.sh | 73 +++++++++++ meta/scripts/lib/tests/run_all.sh | 26 ++++ meta/scripts/lib/tests/test_case_runner.sh | 55 +++++++++ meta/scripts/lib/tests/test_coverage_bar.sh | 63 ++++++++++ meta/scripts/lib/tests/test_jail_setup.sh | 115 ++++++++++++++++++ 9 files changed, 413 insertions(+), 5 deletions(-) create mode 100644 docs/superpowers/contracts/wire-shell-coverage-ratchet-2026-08-23.json create mode 100644 docs/superpowers/evidence/wire-shell-coverage-ratchet-2026-08-23.json create mode 100644 meta/scripts/lib/tests/coverage_tool_harness.sh create mode 100755 meta/scripts/lib/tests/run_all.sh create mode 100755 meta/scripts/lib/tests/test_case_runner.sh create mode 100755 meta/scripts/lib/tests/test_coverage_bar.sh create mode 100755 meta/scripts/lib/tests/test_jail_setup.sh diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f292e5aa..4c801d4b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -134,6 +134,19 @@ repos: types: [shell] require_serial: true exclude: ^(\.git/|phone_focus_mode/lib/tests/|tests/) + - id: shell-coverage-ratchet + name: Every shell library has a test suite beside it + # The ratchet from meta/shell-coverage-allowlist.txt: a lib under a + # lib/ directory needs a tests/run_all.sh, unless it is allowlisted. + # Wired here on 2026-08-23. It existed but was never in pre-commit, + # so it had been silently unenforced -- a new untested lib could land + # freely. That was only affordable to fix once 578c4a6c dropped the + # jail-measured 100% bar: --all went from >600s to 0.31s, and the + # per-file mode this hook uses is milliseconds. + entry: bash meta/scripts/check_shell_coverage.sh + language: system + types: [shell] + exclude: ^third_party/ # =========================================================================== # NOQA BLOCKER - Zero tolerance for noqa/type:ignore suppression comments diff --git a/docs/superpowers/contracts/wire-shell-coverage-ratchet-2026-08-23.json b/docs/superpowers/contracts/wire-shell-coverage-ratchet-2026-08-23.json new file mode 100644 index 00000000..d82ba9db --- /dev/null +++ b/docs/superpowers/contracts/wire-shell-coverage-ratchet-2026-08-23.json @@ -0,0 +1,19 @@ +{ + "title": "Wire the shell coverage ratchet into pre-commit and test the coverage tooling", + "objective": "Make the shell ratchet enforceable at commit time rather than existing but never running, and close the gap where the three libs that build the coverage jail had no tests of their own.", + "acceptance_criteria": [ + "A new shell library in a directory with no tests/run_all.sh is refused by pre-commit.", + "A shell library in a directory that has a suite, or one on the allowlist, is accepted.", + "check_shell_coverage.sh --all reports zero BLOCK entries and completes in under 5 seconds.", + "check_shell_coverage.sh emits no 'unbound variable' error on its failure path.", + "The new suite runs green standalone via bash meta/scripts/lib/tests/run_all.sh.", + "No lint suppression (noqa, shellcheck disable, per-file ignore) is added anywhere.", + "The allowlist is not grown; entries may only be removed." + ], + "out_of_scope": [ + "Restoring the jail-measured 100% bar removed in 578c4a6c.", + "Writing suites for the 8 allowlisted directories that still have none.", + "Changing shell_coverage_jail.sh, which is still used by ci_mirror.sh." + ], + "verifier": "bash meta/scripts/lib/tests/run_all.sh exits 0 with 48 assertions; pre-commit run shell-coverage-ratchet --all-files passes; creating an untested lib under mtk_root/lib and running the hook on it fails with a 'needs: .../tests/run_all.sh' message; shellcheck -x and shfmt -l are clean on every touched file." +} diff --git a/docs/superpowers/evidence/wire-shell-coverage-ratchet-2026-08-23.json b/docs/superpowers/evidence/wire-shell-coverage-ratchet-2026-08-23.json new file mode 100644 index 00000000..d7e64759 --- /dev/null +++ b/docs/superpowers/evidence/wire-shell-coverage-ratchet-2026-08-23.json @@ -0,0 +1,44 @@ +{ + "intent": "Wire check_shell_coverage.sh into pre-commit so the shell ratchet cannot silently rot, and give the coverage tooling itself the suite it was missing.", + "scope": [ + ".pre-commit-config.yaml (new shell-coverage-ratchet hook)", + "meta/scripts/check_shell_coverage.sh (stale message referencing a removed constant)", + "meta/scripts/lib/tests/ (new suite: harness + 3 test files + run_all.sh)" + ], + "changes": [ + "Added the shell-coverage-ratchet hook, types: [shell], excluding third_party/. It was previously never wired, so a new untested shell library could land freely.", + "FIX: check_shell_coverage.sh line 177 interpolated ${COVERAGE_BAR}, which no longer exists after 578c4a6c, producing 'COVERAGE_BAR: unbound variable' on every failure path. The message now describes the presence bar it actually enforces.", + "New suite for the three meta/scripts/lib coverage-tooling libs, which are the one corner of the repo the jail cannot measure itself: 48 assertions over is_covered, build_jail, write_cases, build_mount_script, reconcile_pass_failures and write_case_runner.", + "Assertions on unexpanded ${BASH_SOURCE}/$1 literals compose the dollar sign from an exported DOLLAR constant rather than writing single-quoted literals, so the files are SC2016-clean with no suppressions." + ], + "verification": [ + { + "command": "bash meta/scripts/lib/tests/run_all.sh", + "result": "pass", + "evidence": "48 assertions across 3 files, 0 failed (5 + 26 + 17)" + }, + { + "command": "pre-commit run shell-coverage-ratchet --all-files", + "result": "pass", + "evidence": "Passed in 0.16s; check_shell_coverage.sh --all now reports 0 BLOCK entries, down from 3, and runs in 0.31s" + }, + { + "command": "Created an untested lib at mtk_root/lib/zz_probe.sh and ran the hook on it", + "result": "pass", + "evidence": "Hook failed with 'needs: linux_configuration/scripts/mtk_root/lib/tests/run_all.sh'. Probe file removed; the ratchet is not toothless." + }, + { + "command": "shellcheck -x and shfmt -l over the new files", + "result": "pass", + "evidence": "clean, no noqa/disable directives added" + } + ], + "risks": [ + "The hook now runs on every commit touching a shell file. Measured at 0.16s repo-wide, so the cost is negligible.", + "A lib added to a directory that has no suite is now blocked at commit time. That is the intended ratchet behaviour, and all 8 such directories are already allowlisted." + ], + "rollback": [ + "Remove the shell-coverage-ratchet block from .pre-commit-config.yaml; the gate returns to being unenforced.", + "After rollback, confirm `pre-commit run --all-files` no longer lists the hook." + ] +} diff --git a/meta/scripts/check_shell_coverage.sh b/meta/scripts/check_shell_coverage.sh index f7fa3688..c5bf3a05 100755 --- a/meta/scripts/check_shell_coverage.sh +++ b/meta/scripts/check_shell_coverage.sh @@ -168,16 +168,16 @@ check_files() { return 0 fi - echo "$SCRIPT_NAME: ${#offenders[@]} shell librar(ies) below the coverage bar:" >&2 + echo "$SCRIPT_NAME: ${#offenders[@]} shell librar(ies) without a suite:" >&2 for path in "${offenders[@]}"; do printf ' %s\n' "$path" >&2 printf ' needs: %s/tests/run_all.sh\n' "$(dirname "$path")" >&2 done echo >&2 - echo " A shell library must ship with a suite that drives it to - ${COVERAGE_BAR}% line coverage. Extend" >&2 - echo " the nearest existing tests/ directory rather than cloning a" >&2 - echo " harness -- jscpd fails above 2% duplication." >&2 + echo " A shell library must ship with a suite that sources and" >&2 + echo " exercises it. Extend the nearest existing tests/ directory" >&2 + echo " rather than cloning a harness -- jscpd fails above 2%" >&2 + echo " duplication." >&2 return 1 } diff --git a/meta/scripts/lib/tests/coverage_tool_harness.sh b/meta/scripts/lib/tests/coverage_tool_harness.sh new file mode 100644 index 00000000..e8b069af --- /dev/null +++ b/meta/scripts/lib/tests/coverage_tool_harness.sh @@ -0,0 +1,73 @@ +# shellcheck shell=bash +# lib/tests/coverage_tool_harness.sh — fixture for the shell-coverage tooling. +# +# Sourced, not executed. These three libs are pure file-writers and one +# predicate: given $JAIL (a temp dir) they emit run_cases.sh, cases and +# mounts.sh, and is_covered answers from the filesystem alone. Nothing here +# mounts anything, runs kcov, or needs a namespace -- the jail itself is +# exercised end-to-end by the suites that already run under it. +set -uo pipefail + +PASS=0 +FAIL=0 + +_t_pass() { + PASS=$((PASS + 1)) + printf ' OK: %s\n' "$1" +} + +_t_fail() { + FAIL=$((FAIL + 1)) + printf ' FAIL: %s\n' "$1" +} + +_t_eq() { + if [[ "$2" == "$1" ]]; then + _t_pass "$3" + else + _t_fail "$3 (want '$1', got '$2')" + fi +} + +_t_has() { + if [[ "$1" == *"$2"* ]]; then + _t_pass "$3" + else + _t_fail "$3 (want a substring '$2')" + fi +} + +_t_lacks() { + if [[ "$1" != *"$2"* ]]; then + _t_pass "$3" + else + _t_fail "$3 (did not want substring '$2')" + fi +} + +# A throwaway $JAIL for one test file, removed on exit. The libs write into it +# unconditionally, so every case needs its own clean directory. +_t_new_jail() { + JAIL="$(mktemp -d -t covtool-XXXXXX)" + export JAIL + mkdir -p "$JAIL/trace" +} + +_t_drop_jail() { + if [[ -n "${JAIL:-}" && "$JAIL" == /tmp/covtool-* ]]; then + rm -rf "$JAIL" + fi +} + +# The literal dollar sign, built rather than written. Several assertions here +# check that a file contains an UNEXPANDED ${BASH_SOURCE} or $1, and writing +# that as a single-quoted literal trips SC2016 on every line. Composing the +# needle keeps the intent obvious and the file suppression-free. +DOLLAR='$' +export DOLLAR +readonly DOLLAR + +_t_summary() { + printf '\n%s: %d passed, %d failed\n' "$(basename "$0")" "$PASS" "$FAIL" + [[ $FAIL -eq 0 ]] +} diff --git a/meta/scripts/lib/tests/run_all.sh b/meta/scripts/lib/tests/run_all.sh new file mode 100755 index 00000000..5db8e827 --- /dev/null +++ b/meta/scripts/lib/tests/run_all.sh @@ -0,0 +1,26 @@ +#!/usr/bin/env bash +# lib/tests/run_all.sh — run every shell-coverage tooling test in one go, and +# exit non-zero if any of them fails. +# +# These three libs build and measure the coverage jail, so they are the one +# corner of the repo the jail cannot check itself. The tests are pure: they +# call the file-writing functions against a temp $JAIL and assert on what was +# written. Nothing here mounts, unshares, or runs kcov. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +failed=0 +for test_script in "${SCRIPT_DIR}"/test_*.sh; do + printf '\n### %s\n' "$(basename "$test_script")" + if ! "$test_script"; then + failed=$((failed + 1)) + fi +done + +if [[ $failed -gt 0 ]]; then + printf '\n%d test file(s) FAILED\n' "$failed" + exit 1 +fi + +printf '\nAll shell-coverage tooling test files passed.\n' diff --git a/meta/scripts/lib/tests/test_case_runner.sh b/meta/scripts/lib/tests/test_case_runner.sh new file mode 100755 index 00000000..128f8ac0 --- /dev/null +++ b/meta/scripts/lib/tests/test_case_runner.sh @@ -0,0 +1,55 @@ +#!/usr/bin/env bash +# write_case_runner: the script that executes cases INSIDE the jail. +set -uo pipefail +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=./coverage_tool_harness.sh +. "$HERE/coverage_tool_harness.sh" +# shellcheck source=../shell_coverage_case_runner.sh +. "$HERE/../shell_coverage_case_runner.sh" + +_t_new_jail +write_case_runner +runner="$JAIL/run_cases.sh" + +_t_eq "yes" "$([[ -x "$runner" ]] && echo yes)" "the runner is written executable" +body="$(cat "$runner")" + +# It runs in the child namespace, so it must be syntactically valid on its own. +bash -n "$runner" 2>/dev/null +_t_eq "0" "$?" "the emitted runner parses as bash" + +# Both passes must be present, and they cannot be merged: kcov's ptrace and +# xtrace destroy each other's numbers. +_t_has "$body" "kcov --include-pattern" "the kcov pass invokes kcov with an include pattern" +_t_has "$body" "BASH_ENV=" "the trace pass delivers tracing via BASH_ENV" +_t_lacks "$body" "SHELLOPTS=xtrace" "tracing is NOT delivered via SHELLOPTS (readonly, and dropped when privileged)" + +# fd 199, deliberately high: a subject doing `exec 9>...` would clobber a low fd +# and silently truncate the trace. +_t_has "$body" "BASH_XTRACEFD=199" "the trace uses the high fd 199" +_t_has "$body" '199>>' "and the runner opens 199 itself so the subject cannot swallow it" + +# Every case is bounded and has stdin closed: a subject that reads stdin +# otherwise blocks on the caller's terminal for the whole timeout. +_t_has "$body" "timeout --kill-after=10s" "each case runs under a bounded timeout" +_t_has "$body" ""$tmp/nosuite/lib/thing.sh" +if is_covered "$tmp/nosuite/lib/thing.sh"; then + _t_fail "a lib with no tests/ dir is uncovered" +else + _t_pass "a lib with no tests/ dir is uncovered" +fi + +# A tests/ directory that exists but has no run_all.sh: still uncovered. The +# suite is identified by its runner, not by the directory. +mkdir -p "$tmp/emptytests/lib/tests" +: >"$tmp/emptytests/lib/thing.sh" +: >"$tmp/emptytests/lib/tests/test_thing.sh" +if is_covered "$tmp/emptytests/lib/thing.sh"; then + _t_fail "a tests/ dir without run_all.sh is uncovered" +else + _t_pass "a tests/ dir without run_all.sh is uncovered" +fi + +# The covered case. +mkdir -p "$tmp/good/lib/tests" +: >"$tmp/good/lib/thing.sh" +: >"$tmp/good/lib/tests/run_all.sh" +if is_covered "$tmp/good/lib/thing.sh"; then + _t_pass "a lib beside a tests/run_all.sh is covered" +else + _t_fail "a lib beside a tests/run_all.sh is covered" +fi + +# The runner is looked up beside the LIB, not anywhere up the tree: a suite one +# directory up must not cover a nested lib. +mkdir -p "$tmp/nested/lib/tests" "$tmp/nested/lib/inner" +: >"$tmp/nested/lib/tests/run_all.sh" +: >"$tmp/nested/lib/inner/thing.sh" +if is_covered "$tmp/nested/lib/inner/thing.sh"; then + _t_fail "a suite one level up does not cover a nested lib" +else + _t_pass "a suite one level up does not cover a nested lib" +fi + +# A run_all.sh that is a DIRECTORY is not a runner. +mkdir -p "$tmp/dirrunner/lib/tests/run_all.sh" +: >"$tmp/dirrunner/lib/thing.sh" +if is_covered "$tmp/dirrunner/lib/thing.sh"; then + _t_fail "a directory named run_all.sh is not a suite" +else + _t_pass "a directory named run_all.sh is not a suite" +fi + +_t_summary diff --git a/meta/scripts/lib/tests/test_jail_setup.sh b/meta/scripts/lib/tests/test_jail_setup.sh new file mode 100755 index 00000000..6847f7a5 --- /dev/null +++ b/meta/scripts/lib/tests/test_jail_setup.sh @@ -0,0 +1,115 @@ +#!/usr/bin/env bash +# build_jail, write_cases, build_mount_script and reconcile_pass_failures. +set -uo pipefail +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +# shellcheck source=./coverage_tool_harness.sh +. "$HERE/coverage_tool_harness.sh" + +EXTRA_SHIMS=() +BIND_PATHS=() +CASES=() +# shellcheck source=../shell_coverage_jail_setup.sh +. "$HERE/../shell_coverage_jail_setup.sh" + +# --- build_jail ------------------------------------------------------------- +# It mints its own $JAIL via mktemp, so it is the one function not given one. +EXTRA_SHIMS=(mytool) +build_jail +_t_eq "yes" "$([[ -d "$JAIL/cov" && -d "$JAIL/trace" && -d "$JAIL/bin" ]] && echo yes)" \ + "build_jail creates cov/, trace/ and bin/" + +xtrace="$(cat "$JAIL/xtrace_env.sh")" +_t_has "$xtrace" 'PS4=' "the BASH_ENV file assigns PS4" +_t_has "$xtrace" 'set -x' "the BASH_ENV file re-applies set -x" +# The heredoc is quoted so these reach the traced shell unexpanded; if they had +# been expanded at write time the trace loses its per-line file:line prefix. +_t_has "$xtrace" "${DOLLAR}{BASH_SOURCE}" "BASH_SOURCE is left unexpanded for the traced shell" +_t_has "$xtrace" "${DOLLAR}{LINENO}" "LINENO is left unexpanded for the traced shell" + +_t_eq "yes" "$([[ -x "$JAIL/bin/systemctl" ]] && echo yes)" \ + "a default shim is written and executable" +_t_eq "yes" "$([[ -x "$JAIL/bin/mytool" ]] && echo yes)" \ + "an EXTRA_SHIMS entry is shimmed too" + +# A shim records the call and exits 0, so a subject calling systemctl does not +# die inside the jail. +"$JAIL/bin/systemctl" start nothing.service +_t_has "$(cat "$JAIL/calls.log")" "systemctl start nothing.service" \ + "a shim logs its arguments to calls.log" +_t_eq "0" "$( + "$JAIL/bin/pkill" -9 anything >/dev/null 2>&1 + echo $? +)" \ + "a shim exits 0 rather than failing the subject" + +built_jail="$JAIL" + +# --- write_cases ------------------------------------------------------------ +_t_new_jail +CASES=("" "--flag one" "two") +write_cases +_t_eq "3" "$(wc -l <"$JAIL/cases")" "write_cases writes one line per case" +_t_eq "--flag one" "$(sed -n '2p' "$JAIL/cases")" "a case keeps its arguments verbatim" +_t_eq "" "$(sed -n '1p' "$JAIL/cases")" "the empty case is preserved as a blank line" +_t_drop_jail + +# --- build_mount_script ----------------------------------------------------- +_t_new_jail +BIND_PATHS=(/etc /var/lib) +build_mount_script +mounts="$(cat "$JAIL/mounts.sh")" +_t_has "$mounts" "mkdir -p /etc" "each bind target is mkdir'd first" +_t_has "$mounts" "mount --bind" "each bind emits a mount --bind" +_t_eq "2" "$(grep -c 'mount --bind' "$JAIL/mounts.sh")" "one mount per bind path" +# A failed bind must be fatal: a silent failure sends the subject's writes at +# the REAL path, which is the misdiagnosis the jail exists to prevent. +_t_has "$mounts" "exit 1" "a failed bind exits rather than warning" +# Distinct source dirs, so two binds cannot collide on one mnt dir. +_t_has "$mounts" "mnt0" "the first bind uses mnt0" +_t_has "$mounts" "mnt1" "the second bind uses mnt1" +_t_drop_jail + +# An empty BIND_PATHS is legal and yields an empty script, not an error. +_t_new_jail +BIND_PATHS=() +build_mount_script +_t_eq "0" "$(grep -c 'mount --bind' "$JAIL/mounts.sh" || true)" \ + "no bind paths yields no mounts" +_t_drop_jail + +# --- reconcile_pass_failures ------------------------------------------------ +# Equal failure counts: the trace is complete, so it passes and the kcov file +# becomes the one --fail-on-case-error reads. +_t_new_jail +printf '1\n' >"$JAIL/case_failures.kcov" +printf '1\n' >"$JAIL/case_failures.trace" +(reconcile_pass_failures) >/dev/null 2>&1 +_t_eq "0" "$?" "equal failures in both passes is not an error" +reconcile_pass_failures >/dev/null 2>&1 +_t_eq "yes" "$([[ -f "$JAIL/case_failures" ]] && echo yes)" \ + "the kcov failures are collapsed into case_failures" +_t_drop_jail + +# More trace failures than kcov failures means a PARTIAL trace: it must fail +# loudly, because a short trace under-reports and looks like real uncovered code. +_t_new_jail +: >"$JAIL/case_failures.kcov" +printf '1\n2\n' >"$JAIL/case_failures.trace" +out="$( (reconcile_pass_failures) 2>&1)" +rc=$? +_t_eq "1" "$rc" "a trace pass with MORE failures exits 1" +_t_has "$out" "trace is partial" "and says the trace is partial" +_t_has "$out" "raise --timeout" "and names the usual fix" +_t_drop_jail + +# Neither pass failed: nothing to reconcile and no case_failures file. +_t_new_jail +(reconcile_pass_failures) >/dev/null 2>&1 +_t_eq "0" "$?" "no failure files at all is not an error" +reconcile_pass_failures >/dev/null 2>&1 +_t_eq "no" "$([[ -f "$JAIL/case_failures" ]] && echo yes || echo no)" \ + "a clean run writes no case_failures file" +_t_drop_jail + +rm -rf "$built_jail" +_t_summary