diff --git a/docs/kcov-under-report.md b/docs/kcov-under-report.md index a6c3feeb..05431381 100644 --- a/docs/kcov-under-report.md +++ b/docs/kcov-under-report.md @@ -1,5 +1,12 @@ # kcov under-reports shell coverage: two distinct defects +> **RESOLVED 2026-08-22.** Both defects are fixed; the instrument now +> corrects for them and the numbers below are the *historical* readings that +> motivated the work. Kept because the disproven hypotheses are still worth +> not retrying, and because the two root causes are the kind that silently +> come back. What the fix does, and the three findings that make it work, is +> at the end of this file under "How it was fixed". + Split out of `docs/shell-split-verification.md` on 2026-08-22 to hold both files under the repo's 250-line cap. This is the campaign's instrument problem; read it before trusting any coverage percentage. @@ -97,3 +104,59 @@ surfaced by `--fail-on-case-error`. Temporarily ending the suite with `exit 42` at a chosen point turns "did execution reach here?" into a yes/no the jail will answer. That is what proved the second pattern is a tracing failure and not an aborted suite. + +## How it was fixed (2026-08-22) + +The instrument now runs **two passes over the same cases, in two fresh +namespaces**, and combines them: + +* **denominator** = kcov's instrumentable line set, minus continuation lines + of multi-line quoted arguments (defect (a), detected by tracking quote + state across the file in `meta/scripts/lib/shell_coverage_lines.py`); +* **numerator** = (PS4-traced lines ∪ kcov's own hits) ∩ denominator + (defect (b)). + +The trace cannot supply the denominator. A trace only ever reports lines that +*ran*, so using it for both halves would make every subject 100.00% and gate +on nothing. kcov's line set is what keeps the gate meaningful; kcov's *hits* +are what could not be trusted. + +### Three findings, each of which silently produces a WRONG trace + +1. **kcov and xtrace cannot share a process.** Under `SHELLOPTS=xtrace`, + every line kcov had recorded as `hits="1"` comes back `hits="0"` — + reproduced minimally, A/B, on the same subject. Hence two passes. +2. **`SHELLOPTS` is a readonly variable inside bash.** `export + SHELLOPTS=xtrace` fails with "readonly variable" and tracing never turns + on. It only works placed in the environment *of* the process. +3. **Decisive: bash under `unshare --user --map-root-user` runs in PRIVILEGED + mode and DISCARDS an inherited `PS4`**, falling back to the default `+ `, + while still honouring `SHELLOPTS=xtrace` and `BASH_XTRACEFD`. The trace + then carries no `file:line` prefix at all, so every subject reads as 0% + covered. This is why the trace is delivered through a **`BASH_ENV` file** + that assigns `PS4` and runs `set -x` from *inside* each shell — which also + solves propagation into child processes, since `set -x` does not inherit. + +A fourth trap, in the report rather than the jail: **kcov records +`filename="dwm_config.sh"`, a bare basename, not a path.** The source +location has to come from the trace (which carries `${BASH_SOURCE}`) or from +a repo search; an ambiguous basename yields no exclusion rather than a guess. + +### Measured effect, all through `run_all.sh` + +| lib | before | after | +| --- | --- | --- | +| `dot_resolver_install.sh` | 39/39 = 100.00% | 39/39 = 100.00% (acceptance) | +| `rpi_nc_ca.sh` | 72/73 = 98.63% | **73/73 = 100.00%** | +| `dwm_config.sh` | 35/73 = 47.95% | **65/66 = 98.48%** | +| `rpi_nc_install.sh` | 10/88 = 11.36% | **85/88 = 96.59%** | + +`rpi_nc_ca.sh` reaching 100.00% is the load-bearing check: its missing line +141 was the *cross-process contagion* described above, and the trace pass has +no kcov in it to be contaminated. Nothing went down. + +`dwm_config.sh`'s remaining uncovered line 39 is a `done < <(...)` process +substitution — the other half of defect (a). It is deliberately still in the +denominator: every line removed from a denominator inflates coverage, which +is the fail-open direction, so a second exclusion rule should be justified +per line rather than generalised from one lib. diff --git a/docs/superpowers/contracts/kcov-instrument-two-pass-2026-08-22.json b/docs/superpowers/contracts/kcov-instrument-two-pass-2026-08-22.json new file mode 100644 index 00000000..689cb20c --- /dev/null +++ b/docs/superpowers/contracts/kcov-instrument-two-pass-2026-08-22.json @@ -0,0 +1,18 @@ +{ + "title": "Fix both kcov defects: correct denominator, and replace its numerator with a PS4 trace", + "objective": "Make the shell-coverage instrument report the truth, so the allowlist campaign measures tests rather than tooling. kcov counted continuation lines of multi-line quoted arguments as statements (wrong denominator) and failed to record ordinary statements that provably execute (wrong numerator); the two together held dwm_config.sh at 47.95% and rpi_nc_install.sh at 11.36% regardless of how many tests were written.", + "acceptance_criteria": [ + "dot_resolver_install.sh still measures 39/39 = 100.00% through run_all.sh, gating at --min 100.", + "rpi_nc_ca.sh measures 73/73 = 100.00% -- it was 72/73 = 98.63% only because of cross-process contagion from test_dwm_config.sh, which a kcov-free trace pass cannot suffer.", + "No lib's percentage goes DOWN. A drop means a lost trace, not a stricter instrument.", + "The denominator is never derived from the trace. A PS4 trace reports only lines that ran, so trace-as-denominator would make every subject 100% and gate on nothing; kcov's line set is what keeps the gate meaningful.", + "meta/scripts/lib/shell_coverage_lines.py and meta/scripts/shell_coverage_report.py both hold 100% branch coverage.", + "pre-commit passes on every touched file with no suppression added." + ], + "out_of_scope": [ + "Adding a second denominator-exclusion rule for `done < <(...)` process substitution. Every line removed from a denominator inflates coverage -- the fail-open direction -- so it must be justified per line, not generalised from one lib.", + "Wiring check_shell_coverage.sh into pre-commit/pre-push/CI, which waits for the allowlist to reach zero.", + "The unrelated red `Python tests` CI job (pip resolution-too-deep since 2026-08-17)." + ], + "verifier": "bash meta/scripts/shell_coverage_jail.sh --subject linux_configuration/scripts/single_use/features/lib/tests/run_all.sh $(jail_args) --measure dot_resolver_install.sh --min 100 --fail-on-case-error -- \"\" and the same for rpi_nc_ca.sh (both must exit 0), plus python3 -m pytest python_pkg/tests/test_shell_coverage_lines.py python_pkg/tests/test_shell_coverage_report.py (expects 29 passed), plus pre-commit run --files on the touched files." +} diff --git a/docs/superpowers/evidence/kcov-instrument-two-pass-2026-08-22.json b/docs/superpowers/evidence/kcov-instrument-two-pass-2026-08-22.json new file mode 100644 index 00000000..ebf09594 --- /dev/null +++ b/docs/superpowers/evidence/kcov-instrument-two-pass-2026-08-22.json @@ -0,0 +1,70 @@ +{ + "intent": "Make shell coverage measure the tests instead of the instrument: correct kcov's denominator (continuation lines of multi-line quoted arguments are not statements) and replace its unreliable numerator with a PS4 xtrace, so libs stop being capped at numbers no test can move.", + "scope": [ + "meta/scripts/shell_coverage_jail.sh - runs two passes and reconciles their failures", + "meta/scripts/lib/shell_coverage_case_runner.sh - per-pass case execution; BASH_ENV trace delivery", + "meta/scripts/lib/shell_coverage_jail_setup.sh - trace dir, xtrace_env.sh, reconcile_pass_failures", + "meta/scripts/lib/shell_coverage_lines.py - new: statement/executed line classification", + "meta/scripts/shell_coverage_report.py - union arithmetic and source resolution", + "python_pkg/tests/test_shell_coverage_lines.py, python_pkg/tests/test_shell_coverage_report.py - new tests", + "docs/kcov-under-report.md - records the fix and the three root causes", + "Non-goal: wiring the gate into pre-commit/pre-push/CI (waits for the allowlist to reach zero)", + "Non-goal: excluding `done < <(...)` from the denominator (fail-open direction; needs per-line justification)" + ], + "changes": [ + "Two passes over the same cases in two fresh namespaces: kcov's ptrace and bash's xtrace are mutually exclusive, measured A/B -- under SHELLOPTS=xtrace every kcov hit collapses from 1 to 0. Pass one supplies the instrumentable line set, pass two the executed set.", + "Coverage = (PS4-traced lines UNION kcov hits) INTERSECT (kcov line set MINUS continuation lines). The trace deliberately does not supply the denominator: it can only report lines that ran, so trace-as-denominator would make every subject 100% and gate on nothing.", + "Trace delivered via a BASH_ENV file that assigns PS4 and runs `set -x` inside each shell. Required because bash under `unshare --map-root-user` runs privileged and DISCARDS an inherited PS4 (falling back to the default '+ ', losing the file:line prefix entirely), and because `set -x` does not propagate to child processes while SHELLOPTS is readonly inside bash.", + "Continuation lines detected by tracking single/double quote state across the source, honouring backslash escapes in double quotes only and treating an unquoted # as end-of-line.", + "Source path resolved from the trace's ${BASH_SOURCE} or a unique repo search, because kcov records only a bare basename; an ambiguous basename yields no exclusion rather than a wrong guess.", + "The trace pass failing more cases than the kcov pass is a hard error: a partially-traced case under-reports and looks exactly like the defect being fixed.", + "Traced lines absent from kcov's line set are counted and reported rather than silently dropped -- a non-zero count is a third defect the design cannot correct for." + ], + "verification": [ + { + "command": "shell_coverage_jail.sh --measure dot_resolver_install.sh --min 100 --fail-on-case-error (through run_all.sh)", + "result": "pass", + "evidence": "dot_resolver_install.sh: 39/39 lines = 100.00%, exit 0. Acceptance baseline unchanged by the instrument change." + }, + { + "command": "shell_coverage_jail.sh --measure rpi_nc_ca.sh --min 100 --fail-on-case-error (through run_all.sh)", + "result": "pass", + "evidence": "rpi_nc_ca.sh: 73/73 lines = 100.00%, exit 0 -- was 72/73 = 98.63%. Line 141 was lost to cross-process contagion from test_dwm_config.sh; the trace pass has no kcov in it to contaminate. This is the discriminating check that the numerator fix landed." + }, + { + "command": "shell_coverage_jail.sh --measure dwm_config.sh (through run_all.sh)", + "result": "pass", + "evidence": "35/73 = 47.95% -> 65/66 = 98.48%. Denominator lost exactly the 7 continuation lines 64-66 and 98-101; numerator rose from 35 to 65. Remaining uncovered line 39 is a `done < <(...)`, left in the denominator deliberately." + }, + { + "command": "shell_coverage_jail.sh --measure rpi_nc_install.sh (through run_all.sh)", + "result": "pass", + "evidence": "10/88 = 11.36% -> 85/88 = 96.59%. The lib was never test-limited; its 29 assertions already passed." + }, + { + "command": "python3 -m pytest python_pkg/tests/test_shell_coverage_lines.py python_pkg/tests/test_shell_coverage_report.py", + "result": "pass", + "evidence": "29 passed." + }, + { + "command": "python3 -m coverage run --branch --source=meta/scripts -m pytest (the two new test modules); coverage report -m", + "result": "pass", + "evidence": "meta/scripts/lib/shell_coverage_lines.py 53 stmts 38 branches 100%; meta/scripts/shell_coverage_report.py 61 stmts 28 branches 100%." + }, + { + "command": "pre-commit run --files (the seven touched code files)", + "result": "pass", + "evidence": "All hooks pass. Three real lint findings were fixed at source, not suppressed: EXE001 (shebang without exec bit), PLR2004 (magic argv index named _ARGV_WITH_TRACE), E501 (long test signatures wrapped)." + } + ], + "risks": [ + "Every measurement now runs the suite twice, so an --all sweep takes roughly double the wall time. That is why the gate belongs in CI rather than a commit hook.", + "xtrace is slower than an untraced run, so a case near its --timeout under kcov can exceed it under trace. This fails loudly via the pass-failure reconciliation rather than silently under-reporting.", + "A subject that runs `set +x`, inspects $-, or opens fd 199 would corrupt its own trace. Neither construct exists in any current subject (grepped), and fd 199 was chosen over a low fd for this reason.", + "The continuation-line detector is a quote-state scanner, not a bash parser. Over-detection would inflate coverage; it was validated against dwm_config.sh, where it flags exactly the 7 known lines and nothing else." + ], + "rollback": [ + "git revert the commit; the previous instrument is self-contained in the same five files.", + "After rollback, re-measure dot_resolver_install.sh (expect 39/39) and rpi_nc_ca.sh (expect the old 72/73 = 98.63%) to confirm the old behaviour is back, and restore the allowlist entries for any lib cleared on the strength of the new numbers." + ] +} diff --git a/meta/scripts/lib/shell_coverage_case_runner.sh b/meta/scripts/lib/shell_coverage_case_runner.sh index a5265ed0..e97c6e45 100755 --- a/meta/scripts/lib/shell_coverage_case_runner.sh +++ b/meta/scripts/lib/shell_coverage_case_runner.sh @@ -11,6 +11,41 @@ # 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. +# +# TWO PASSES, and they cannot be merged. kcov instruments via ptrace; bash's +# xtrace is a shell feature. Running both over one process was measured to +# destroy kcov's numerator outright -- with SHELLOPTS=xtrace set, every line +# kcov had recorded as hits="1" comes back hits="0". So: +# +# pass "kcov" -> the instrumentable LINE SET (the denominator) plus kcov's +# own hit counts, which are correct for some lines and +# missing for others (defect (b) in docs/kcov-under-report.md). +# pass "trace" -> the EXECUTED line set, from a PS4 xtrace. This is the +# authoritative numerator; kcov's hits are unioned in so a +# line either instrument saw counts as covered. +# +# Tracing is delivered via BASH_ENV, not SHELLOPTS/PS4, and the reason is +# measured, not stylistic. Three findings, each of which yields a silently +# WRONG trace rather than an error: +# +# 1. `set -x` does NOT reach child bash processes, and the suites drive +# their subjects several processes deep. +# 2. SHELLOPTS is a READONLY variable inside bash, so `export +# SHELLOPTS=xtrace` fails with "readonly variable" and tracing never +# turns on. It only works placed in the environment OF the process. +# 3. Decisive: bash started under `unshare --user --map-root-user` runs in +# PRIVILEGED mode (uid != euid) and DISCARDS an inherited PS4, falling +# back to the default "+ " -- while still honouring SHELLOPTS=xtrace. +# The trace then carries no file:line prefix at all and every subject +# reads as 0% covered. +# +# BASH_ENV survives all three: every non-interactive bash sources it at +# startup, so PS4 is ASSIGNED from inside the shell (where privileged mode +# cannot strip it) and `set -x` is re-applied per process, including children. +# +# BASH_XTRACEFD uses fd 199, not a low fd: subjects and harnesses open their +# own descriptors, and a subject doing `exec 9>...` would silently clobber the +# trace mid-run and truncate it. # ============================================================================ write_case_runner() { @@ -26,6 +61,7 @@ subject_dir="$2" subject_base="$3" measure_base="$4" case_timeout="$5" +pass="$6" . "$jail/mounts.sh" export PATH="$jail/bin:/usr/bin:/bin" export HOME="$jail/home" @@ -33,22 +69,40 @@ USER="${USER:-root}" export USER export SUDO_USER="$USER" cd "$subject_dir" || exit 1 +case_index=0 while IFS= read -r line; do read -r -a case_args <<<"$line" + case_index=$((case_index + 1)) # 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 /dev/null 2>&1 /dev/null once cost a bogus + # "0 lines traced" reading. + timeout --kill-after=10s "$case_timeout" \ + env "BASH_XTRACEFD=199" "BASH_ENV=$jail/xtrace_env.sh" \ + "./$subject_base" ${case_args[@]+"${case_args[@]}"} \ + >/dev/null 2>&1 >"$jail/trace/case.$case_index" + rc=$? + fi if [[ $rc -eq 124 || $rc -eq 137 ]]; then printf 'warn: case timed out after %s: %s\n' "$case_timeout" "$line" >&2 - printf '%s\n' "$rc" >>"$jail/case_failures" + printf '%s\n' "$rc" >>"$jail/case_failures.$pass" 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 @@ -56,7 +110,7 @@ while IFS= read -r line; do # 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" + printf '%s\n' "$rc" >>"$jail/case_failures.$pass" fi done <"$jail/cases" INNER diff --git a/meta/scripts/lib/shell_coverage_jail_setup.sh b/meta/scripts/lib/shell_coverage_jail_setup.sh index 3abc60a8..a7943f35 100755 --- a/meta/scripts/lib/shell_coverage_jail_setup.sh +++ b/meta/scripts/lib/shell_coverage_jail_setup.sh @@ -17,7 +17,23 @@ readonly DEFAULT_SHIMS=( build_jail() { JAIL="$(mktemp -d)" - mkdir -p "$JAIL/bin" "$JAIL/cov" "$JAIL/home" "$JAIL/etc" + # cov holds kcov's output (the line SET); trace holds the PS4 xtrace + # output (the executed-line set). They are two SEPARATE passes because + # kcov's ptrace and SHELLOPTS=xtrace cannot share a process -- measured: + # under xtrace every kcov hit count collapses to 0. + mkdir -p "$JAIL/bin" "$JAIL/cov" "$JAIL/trace" "$JAIL/home" "$JAIL/etc" + + # Sourced by every non-interactive bash in the trace pass (via BASH_ENV). + # PS4 must be ASSIGNED here rather than inherited: bash under + # `unshare --map-root-user` runs privileged and drops an inherited PS4, + # which silently costs the trace its file:line prefix. `set -x` is + # likewise re-applied per process, which is what reaches child shells. + # Quoted heredoc: ${BASH_SOURCE}/${LINENO} must reach the traced shell + # unexpanded so they evaluate per traced line. + cat >"$JAIL/xtrace_env.sh" <<'XTRACE' +PS4='+PS4:${BASH_SOURCE}:${LINENO} ' +set -x +XTRACE local tool for tool in "${DEFAULT_SHIMS[@]}" ${EXTRA_SHIMS[@]+"${EXTRA_SHIMS[@]}"}; do @@ -107,3 +123,29 @@ build_mount_script() { done } >"$JAIL/mounts.sh" } + +# A case that passes under kcov but fails under xtrace yields a PARTIAL trace, +# which under-reports and looks exactly like the defect the trace pass exists +# to fix. xtrace is slower, so a tight --timeout is the usual cause. Fail +# loudly rather than silently measuring less. +# +# Also collapses the two per-pass failure files into the one --fail-on-case-error +# reads, so that flag keeps its original meaning. +reconcile_pass_failures() { + local kcov_fail=0 trace_fail=0 + if [[ -s "$JAIL/case_failures.kcov" ]]; then + kcov_fail="$(wc -l <"$JAIL/case_failures.kcov")" + fi + if [[ -s "$JAIL/case_failures.trace" ]]; then + trace_fail="$(wc -l <"$JAIL/case_failures.trace")" + fi + if [[ $trace_fail -gt $kcov_fail ]]; then + printf 'Error: trace pass had %d failing case(s) vs %d under kcov; the\n' \ + "$trace_fail" "$kcov_fail" >&2 + printf ' trace is partial. xtrace is slower -- raise --timeout.\n' >&2 + exit 1 + fi + if [[ -s "$JAIL/case_failures.kcov" ]]; then + cp "$JAIL/case_failures.kcov" "$JAIL/case_failures" + fi +} diff --git a/meta/scripts/lib/shell_coverage_lines.py b/meta/scripts/lib/shell_coverage_lines.py new file mode 100755 index 00000000..af4c2e5c --- /dev/null +++ b/meta/scripts/lib/shell_coverage_lines.py @@ -0,0 +1,109 @@ +#!/usr/bin/env python3 +"""Line classification for shell coverage: what is a statement, and what ran. + +Split out of ``shell_coverage_report.py`` to keep both files under the repo's +250-line cap. + +Two independent instruments feed the report, because neither is correct alone +(see ``docs/kcov-under-report.md``): + +* kcov supplies the *line set* -- which lines are instrumentable at all. Its + hit counts are unreliable: ordinary statements execute without being + recorded (defect (b)). +* a PS4 xtrace supplies the *executed* set. A trace can only ever report lines + that ran, so it cannot supply a denominator -- using it for both halves + would make every subject 100% and gate on nothing. + +So: denominator = kcov's line set minus non-statements; numerator = +(traced lines | kcov's hits) & denominator. +""" + +from __future__ import annotations + +from pathlib import Path +import re + +# A PS4 trace line looks like: "+PS4:/abs/path/to/lib.sh:42 some command" +# The leading '+' repeats with nesting depth, so it is matched loosely. +_TRACE_RE = re.compile(r"\+*PS4:(?P[^:]*):(?P\d+)") + + +def traced_lines(trace_dir: Path, subject: str) -> set[int]: + """Return the line numbers of *subject* seen executing in a PS4 trace.""" + seen: set[int] = set() + if not trace_dir.is_dir(): + return seen + for trace_file in sorted(trace_dir.iterdir()): + if not trace_file.is_file(): + continue + # errors="replace": a trace interleaves writes from concurrent + # processes and can split a UTF-8 sequence. A decode error must not + # discard the whole file, which would read as zero coverage. + text = trace_file.read_text(encoding="utf-8", errors="replace") + for match in _TRACE_RE.finditer(text): + if Path(match.group("path")).name == subject: + seen.add(int(match.group("line"))) + return seen + + +def traced_paths(trace_dir: Path, subject: str) -> list[Path]: + """Return the distinct source paths a PS4 trace recorded for *subject*. + + kcov's XML carries only a basename, so the trace is the one place the + subject's real location is written down. + """ + found: list[Path] = [] + if not trace_dir.is_dir(): + return found + for trace_file in sorted(trace_dir.iterdir()): + if not trace_file.is_file(): + continue + text = trace_file.read_text(encoding="utf-8", errors="replace") + for match in _TRACE_RE.finditer(text): + path = Path(match.group("path")) + if path.name == subject and path not in found: + found.append(path) + return found + + +def continuation_lines(source: Path) -> set[int]: + """Return lines that are *inside* a multi-line quoted argument. + + kcov counts these as instrumentable statements that never run, which + inflates the denominator and caps a subject's coverage below 100% no + matter what the tests do (defect (a)). A `bash -x` tracer never reports + them as executable at all, which is the correct reading: they are data + inside an argument, not statements. + + Detected by tracking quote state across the file: any line that *begins* + while a quote opened on an earlier line is still open is a continuation. + Only the opening line of such a construct is a statement. + """ + inside: set[int] = set() + quote: str | None = None + for number, raw in enumerate( + source.read_text(encoding="utf-8", errors="replace").splitlines(), start=1 + ): + if quote is not None: + inside.add(number) + index = 0 + while index < len(raw): + char = raw[index] + if quote is None: + if char == "#": + # A comment outside quotes ends the line's significance. + break + if char == "\\": + index += 2 + continue + if char in ("'", '"'): + quote = char + elif char == quote: + quote = None + elif quote == '"' and char == "\\": + # Only double quotes honour backslash escapes; inside single + # quotes a backslash is a literal character. + index += 2 + continue + index += 1 + return inside diff --git a/meta/scripts/shell_coverage_jail.sh b/meta/scripts/shell_coverage_jail.sh index 04255f60..c18676ce 100755 --- a/meta/scripts/shell_coverage_jail.sh +++ b/meta/scripts/shell_coverage_jail.sh @@ -23,13 +23,11 @@ # subject under `set -e` long before the interesting code. # 3. Mount only what the subject WRITES; preserve what it READS. A blanket # jail that masked /usr/local/bin cut pacman_wrapper.sh from 75.00% to -# 28.95%, because the wrapper sources its sibling libs from there and -# took its "libraries missing" escape hatch. An over-aggressive mount -# looks exactly like unreachable code. +# 28.95%: the wrapper sources its siblings from there and took its +# "libraries missing" escape hatch. Over-mounting looks like dead code. # -# Entry scripts must be executed in place: they resolve their libs via -# BASH_SOURCE and common.sh via `readlink -f "$0"`, so staging a copy into a -# temp dir (which works fine for a sourced lib) breaks them. +# Entry scripts must be executed in place: they resolve libs via BASH_SOURCE +# and common.sh via `readlink -f "$0"`, so staging a copy breaks them. # ============================================================================ set -euo pipefail @@ -213,9 +211,18 @@ main() { done } >>"$JAIL/mounts.sh" - unshare --user --map-root-user --mount --fork \ - "$JAIL/run_cases.sh" "$JAIL" "$subject_dir" "$subject_base" "$measure_base" \ - "$CASE_TIMEOUT" + # Two passes, two fresh namespaces; they cannot be merged (kcov's ptrace + # and xtrace are mutually exclusive -- measured: under xtrace every kcov + # hit collapses to 0). "kcov" gives the line set, "trace" gives the truth + # about which of those lines ran. See lib/shell_coverage_case_runner.sh. + local pass + for pass in kcov trace; do + unshare --user --map-root-user --mount --fork \ + "$JAIL/run_cases.sh" "$JAIL" "$subject_dir" "$subject_base" \ + "$measure_base" "$CASE_TIMEOUT" "$pass" + done + + reconcile_pass_failures local tree_after tree_after="$(cd "$REPO_ROOT" && git status --porcelain)" @@ -229,7 +236,7 @@ main() { # 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" || report_rc=$? + "$JAIL/cov" "$measure_base" "$MIN_PERCENT" "$JAIL/trace" || 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' \ diff --git a/meta/scripts/shell_coverage_report.py b/meta/scripts/shell_coverage_report.py index 3a256ce1..5cfb24fc 100755 --- a/meta/scripts/shell_coverage_report.py +++ b/meta/scripts/shell_coverage_report.py @@ -5,46 +5,119 @@ Reads kcov's cobertura-style ``cov.xml`` rather than its ``coverage.json``: the JSON summary carries only a percentage, so it cannot say which lines are missing, which is the only part that helps close a gap. +kcov alone is not trustworthy (``docs/kcov-under-report.md``): it counts +continuation lines of multi-line quoted arguments as statements that never run +(wrong denominator), and it fails to record ordinary statements that provably +execute (wrong numerator). Both are corrected here -- the denominator by +excluding non-statements, the numerator by unioning in a PS4 xtrace captured +in a second pass. + Exits 1 when coverage is below the requested minimum, so the caller can use it as a gate. """ from __future__ import annotations +from importlib.util import module_from_spec, spec_from_file_location from pathlib import Path import sys from defusedxml import ElementTree +# Loaded by path rather than imported: meta/scripts/lib is not a package on +# sys.path, and a sys.path.insert would need the import to sit below it, +# which is an E402 the repo will not suppress. +_LINES_PATH = Path(__file__).resolve().parent / "lib" / "shell_coverage_lines.py" +_SPEC = spec_from_file_location("shell_coverage_lines", _LINES_PATH) +if _SPEC is None or _SPEC.loader is None: # pragma: no cover - unreachable + raise ImportError(_LINES_PATH) +_LINES = module_from_spec(_SPEC) +_SPEC.loader.exec_module(_LINES) +continuation_lines = _LINES.continuation_lines +traced_lines = _LINES.traced_lines -def _measure(out_dir: Path, subject: str) -> tuple[int, int, list[str]]: - """Return (covered, total, uncovered line numbers) for *subject*. + +def _kcov_lines(out_dir: Path, subject: str) -> dict[int, int]: + """Return kcov's {line: hits} for *subject*. kcov writes one report per instrumented process. A harness that stages the subject into a temp dir produces several, so the entries are merged: a line counts as covered if any process hit it. """ - hits: dict[str, int] = {} + hits: dict[int, int] = {} for xml_path in out_dir.rglob("cov.xml"): for cls in ElementTree.parse(xml_path).iter("class"): - filename = cls.get("filename", "") - if Path(filename).name != subject: + if Path(cls.get("filename", "")).name != subject: continue for line in cls.iter("line"): number = line.get("number") if number is None: continue - hits[number] = hits.get(number, 0) + int(line.get("hits", "0")) + key = int(number) + hits[key] = hits.get(key, 0) + int(line.get("hits", "0")) + return hits - uncovered = sorted((n for n, h in hits.items() if h == 0), key=int) - return len(hits) - len(uncovered), len(hits), uncovered + +def _find_source(subject: str, trace_dir: Path | None) -> Path | None: + """Locate *subject*'s source file so its non-statements can be excluded. + + kcov records `filename="dwm_config.sh"` -- a bare BASENAME, not a path -- + so the XML cannot supply the source location. The PS4 trace can: it + carries ${BASH_SOURCE}, an absolute path, for every line it reports. + Falling back to a repo search keeps the exclusion working for a subject + whose trace is empty. + """ + if trace_dir is not None: + for candidate in _LINES.traced_paths(trace_dir, subject): + if candidate.is_file(): + return candidate + matches = sorted(Path(__file__).resolve().parents[2].rglob(subject)) + real = [m for m in matches if m.is_file() and ".git" not in m.parts] + # Exactly one match, or the exclusion would be applied from the wrong + # file. Ambiguity is reported by the caller as "no exclusions" rather + # than silently guessing, which could only inflate coverage. + return real[0] if len(real) == 1 else None + + +def _measure( + out_dir: Path, subject: str, trace_dir: Path | None +) -> tuple[int, int, list[str], int]: + """Return (covered, total, uncovered line numbers, off-set trace count).""" + hits = _kcov_lines(out_dir, subject) + if not hits: + return 0, 0, [], 0 + + source = _find_source(subject, trace_dir) + excluded = continuation_lines(source) if source is not None else set() + denominator = set(hits) - excluded + + executed = {line for line, count in hits.items() if count > 0} + traced: set[int] = set() + if trace_dir is not None: + traced = traced_lines(trace_dir, subject) + + # Lines the trace saw executing that kcov never listed as instrumentable. + # Dropped from the numerator (they are not in the denominator), but + # surfaced: a non-zero count means kcov's LINE SET is wrong too, which is + # a third defect this design cannot correct for. + off_set = len(traced - set(hits) - excluded) + + covered_set = (executed | traced) & denominator + uncovered = sorted(denominator - covered_set) + return len(covered_set), len(denominator), [str(n) for n in uncovered], off_set + + +# argv: []. The trace dir is optional +# so the report still runs against a kcov-only output directory. +_ARGV_WITH_TRACE = 5 def main() -> None: """Print the coverage summary and gate on the minimum percentage.""" out_dir, subject, minimum = Path(sys.argv[1]), sys.argv[2], float(sys.argv[3]) + trace_dir = Path(sys.argv[4]) if len(sys.argv) >= _ARGV_WITH_TRACE else None - covered, total, uncovered = _measure(out_dir, subject) + covered, total, uncovered, off_set = _measure(out_dir, subject, trace_dir) if total == 0: sys.exit( f"{subject}: kcov instrumented no lines. It was probably invoked as " @@ -58,6 +131,10 @@ def main() -> None: sys.stdout.write(f"{subject}: {covered}/{total} lines = {percent:.2f}%\n") if uncovered: sys.stdout.write(f" uncovered: {', '.join(uncovered)}\n") + if off_set: + sys.stdout.write( + f" note: {off_set} traced line(s) are absent from kcov's line set\n" + ) if percent < minimum: sys.exit(f"coverage {percent:.2f}% is below the required {minimum:.2f}%") diff --git a/python_pkg/tests/test_shell_coverage_lines.py b/python_pkg/tests/test_shell_coverage_lines.py new file mode 100644 index 00000000..4ad72ab3 --- /dev/null +++ b/python_pkg/tests/test_shell_coverage_lines.py @@ -0,0 +1,172 @@ +"""Tests for meta/scripts/lib/shell_coverage_lines.py. + +This module decides what counts as a statement and what counts as executed, +so both of its failure modes are silent and one-directional: + +* over-detecting continuation lines shrinks the denominator and INFLATES + coverage, which is the fail-open direction -- it could promote an untested + lib off the allowlist; +* losing trace lines shrinks the numerator, which merely keeps a lib on the + allowlist that deserves to come off. + +The first is the one worth pinning down hardest, so the quote-state cases +below assert exact line sets rather than counts. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +if TYPE_CHECKING: + from types import ModuleType + +_ROOT = Path(__file__).resolve().parents[2] +_SCRIPT = _ROOT / "meta" / "scripts" / "lib" / "shell_coverage_lines.py" + + +def _load() -> ModuleType: + """Import the module by path; it lives outside any importable package.""" + spec = importlib.util.spec_from_file_location("shell_coverage_lines", _SCRIPT) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(name="mod") +def _mod() -> ModuleType: + return _load() + + +def _write(tmp_path: Path, name: str, text: str) -> Path: + path = tmp_path / name + path.write_text(text, encoding="utf-8") + return path + + +# -------------------------------------------------------------------------- +# continuation_lines +# -------------------------------------------------------------------------- + + +def test_single_line_quotes_are_not_continuations( + mod: ModuleType, tmp_path: Path +) -> None: + """A quote opened and closed on one line leaves no continuation.""" + src = _write(tmp_path, "a.sh", "echo 'hello'\necho \"world\"\n") + assert mod.continuation_lines(src) == set() + + +def test_multiline_single_quote_marks_inner_lines( + mod: ModuleType, tmp_path: Path +) -> None: + """This is the real dwm_config.sh perl-block shape: only the inner lines.""" + src = _write(tmp_path, "a.sh", "perl -pe '\ns/a/b/;\ns/c/d/;\n' file\necho after\n") + # Line 1 opens the quote and IS a statement; 2-4 are inside it; 5 is after. + assert mod.continuation_lines(src) == {2, 3, 4} + + +def test_multiline_double_quote_marks_inner_lines( + mod: ModuleType, tmp_path: Path +) -> None: + """Double quotes span lines the same way single quotes do.""" + src = _write(tmp_path, "a.sh", 'x="\nline two\n"\necho after\n') + assert mod.continuation_lines(src) == {2, 3} + + +def test_comment_outside_quotes_ends_the_line(mod: ModuleType, tmp_path: Path) -> None: + """An apostrophe inside a comment must not open a quote state.""" + src = _write(tmp_path, "a.sh", "# don't be fooled\necho after\n") + assert mod.continuation_lines(src) == set() + + +def test_backslash_escape_outside_quotes_is_skipped( + mod: ModuleType, tmp_path: Path +) -> None: + """An escaped quote outside quotes does not open a quote state.""" + src = _write(tmp_path, "a.sh", "echo \\'\necho after\n") + assert mod.continuation_lines(src) == set() + + +def test_backslash_escape_inside_double_quotes(mod: ModuleType, tmp_path: Path) -> None: + """An escaped double quote inside double quotes does not close them.""" + src = _write(tmp_path, "a.sh", 'x="a\\"b\nstill inside\n"\n') + assert mod.continuation_lines(src) == {2, 3} + + +def test_backslash_inside_single_quotes_is_literal( + mod: ModuleType, tmp_path: Path +) -> None: + """Single quotes honour no escapes, so a trailing backslash closes nothing.""" + src = _write(tmp_path, "a.sh", "x='a\\'\necho after\n") + assert mod.continuation_lines(src) == set() + + +def test_hash_inside_quotes_is_not_a_comment(mod: ModuleType, tmp_path: Path) -> None: + """A # inside an open quote must not be treated as a comment.""" + src = _write(tmp_path, "a.sh", "x='# not a comment\nstill inside\n'\n") + assert mod.continuation_lines(src) == {2, 3} + + +# -------------------------------------------------------------------------- +# traced_lines / traced_paths +# -------------------------------------------------------------------------- + + +def test_traced_lines_missing_dir_is_empty(mod: ModuleType, tmp_path: Path) -> None: + """A missing trace dir yields nothing rather than raising.""" + assert mod.traced_lines(tmp_path / "absent", "x.sh") == set() + assert mod.traced_paths(tmp_path / "absent", "x.sh") == [] + + +def test_traced_lines_filters_by_basename(mod: ModuleType, tmp_path: Path) -> None: + """Only the subject's own lines are collected.""" + trace = tmp_path / "trace" + trace.mkdir() + (trace / "case.1").write_text( + "+PS4:/a/b/subject.sh:12 cmd\n" + "++PS4:/a/b/subject.sh:15 cmd\n" + "+PS4:/a/b/other.sh:99 cmd\n", + encoding="utf-8", + ) + assert mod.traced_lines(trace, "subject.sh") == {12, 15} + + +def test_traced_lines_skips_subdirectories(mod: ModuleType, tmp_path: Path) -> None: + """A directory inside the trace dir is not read as a trace file.""" + trace = tmp_path / "trace" + (trace / "nested").mkdir(parents=True) + (trace / "case.1").write_text("+PS4:/a/subject.sh:3 cmd\n", encoding="utf-8") + assert mod.traced_lines(trace, "subject.sh") == {3} + + +def test_traced_lines_survives_invalid_utf8(mod: ModuleType, tmp_path: Path) -> None: + """A split multi-byte sequence must not discard the whole trace.""" + trace = tmp_path / "trace" + trace.mkdir() + (trace / "case.1").write_bytes(b"+PS4:/a/subject.sh:7 cmd\n\xff\xfe\n") + assert mod.traced_lines(trace, "subject.sh") == {7} + + +def test_traced_paths_skips_subdirectories(mod: ModuleType, tmp_path: Path) -> None: + """A directory inside the trace dir is not read when resolving paths.""" + trace = tmp_path / "trace" + (trace / "nested").mkdir(parents=True) + (trace / "case.1").write_text("+PS4:/a/subject.sh:3 cmd\n", encoding="utf-8") + assert mod.traced_paths(trace, "subject.sh") == [Path("/a/subject.sh")] + + +def test_traced_paths_deduplicates(mod: ModuleType, tmp_path: Path) -> None: + """The same source path reported many times yields one entry.""" + trace = tmp_path / "trace" + trace.mkdir() + (trace / "case.1").write_text( + "+PS4:/a/subject.sh:1 c\n+PS4:/a/subject.sh:2 c\n+PS4:/b/other.sh:2 c\n", + encoding="utf-8", + ) + assert mod.traced_paths(trace, "subject.sh") == [Path("/a/subject.sh")] diff --git a/python_pkg/tests/test_shell_coverage_report.py b/python_pkg/tests/test_shell_coverage_report.py new file mode 100644 index 00000000..94ff60be --- /dev/null +++ b/python_pkg/tests/test_shell_coverage_report.py @@ -0,0 +1,233 @@ +"""Tests for meta/scripts/shell_coverage_report.py. + +The report combines two instruments that are each wrong on their own +(``docs/kcov-under-report.md``): kcov supplies the line SET but under-reports +hits, and a PS4 trace supplies the executed set but cannot see a line that +never ran. The arithmetic here is what makes the shell-coverage gate mean +something, so the tests below pin the fail-open direction hardest: a trace +must never be able to add a line that is not in the denominator. +""" + +from __future__ import annotations + +import importlib.util +from pathlib import Path +from typing import TYPE_CHECKING + +import pytest + +if TYPE_CHECKING: + from types import ModuleType + +_ROOT = Path(__file__).resolve().parents[2] +_SCRIPT = _ROOT / "meta" / "scripts" / "shell_coverage_report.py" + +_XML = """ + + +{lines} + + +""" + + +def _load() -> ModuleType: + """Import the script by path; it lives outside any importable package.""" + spec = importlib.util.spec_from_file_location("shell_coverage_report", _SCRIPT) + assert spec is not None + assert spec.loader is not None + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +@pytest.fixture(name="mod") +def _mod() -> ModuleType: + return _load() + + +def _cov(tmp_path: Path, name: str, hits: dict[int, int]) -> Path: + """Write a minimal kcov cov.xml tree for *name*.""" + out = tmp_path / "cov" / "proc" + out.mkdir(parents=True) + lines = "".join(f'' for n, h in sorted(hits.items())) + (out / "cov.xml").write_text(_XML.format(name=name, lines=lines), encoding="utf-8") + return tmp_path / "cov" + + +def _trace(tmp_path: Path, source: Path, numbers: list[int]) -> Path: + trace = tmp_path / "trace" + trace.mkdir() + (trace / "case.1").write_text( + "".join(f"+PS4:{source}:{n} cmd\n" for n in numbers), encoding="utf-8" + ) + return trace + + +def test_kcov_hits_alone_are_used_without_a_trace( + mod: ModuleType, tmp_path: Path +) -> None: + """With no trace dir the result is kcov's own reading.""" + cov = _cov(tmp_path, "s.sh", {1: 1, 2: 0}) + covered, total, uncovered, off_set = mod._measure(cov, "s.sh", None) + assert (covered, total, uncovered, off_set) == (1, 2, ["2"], 0) + + +def test_trace_rescues_a_line_kcov_missed(mod: ModuleType, tmp_path: Path) -> None: + """Defect (b): a line kcov reports as 0 hits but that provably ran.""" + source = tmp_path / "s.sh" + source.write_text("echo one\necho two\n", encoding="utf-8") + cov = _cov(tmp_path, "s.sh", {1: 1, 2: 0}) + trace = _trace(tmp_path, source, [2]) + covered, total, uncovered, _ = mod._measure(cov, "s.sh", trace) + assert (covered, total, uncovered) == (2, 2, []) + + +def test_continuation_lines_leave_the_denominator( + mod: ModuleType, tmp_path: Path +) -> None: + """Defect (a): inner lines of a multi-line quoted argument are not statements.""" + source = tmp_path / "s.sh" + # The real dwm_config.sh shape: the closing quote shares a line with the + # rest of the command, so only the middle line is a continuation. + source.write_text("perl -pe '\ns/a/b/;\n' f\necho after\n", encoding="utf-8") + cov = _cov(tmp_path, "s.sh", {1: 1, 2: 0, 4: 1}) + # The trace must be non-empty: it is what tells the report where the + # source lives, since kcov's XML carries only a basename. + trace = _trace(tmp_path, source, [1, 4]) + covered, total, uncovered, _ = mod._measure(cov, "s.sh", trace) + # Line 2 is inside the quote, so the denominator is 1 and 4 only. + assert (covered, total, uncovered) == (2, 2, []) + + +def test_traced_line_outside_the_line_set_is_counted_not_credited( + mod: ModuleType, tmp_path: Path +) -> None: + """A traced line kcov never listed cannot inflate coverage, but is reported.""" + source = tmp_path / "s.sh" + source.write_text("echo one\necho two\necho three\n", encoding="utf-8") + cov = _cov(tmp_path, "s.sh", {1: 1}) + trace = _trace(tmp_path, source, [1, 3]) + covered, total, _, off_set = mod._measure(cov, "s.sh", trace) + assert (covered, total, off_set) == (1, 1, 1) + + +def test_empty_kcov_output_reports_zero_total(mod: ModuleType, tmp_path: Path) -> None: + """No instrumented lines is a distinct, reportable state.""" + cov = _cov(tmp_path, "other.sh", {1: 1}) + assert mod._measure(cov, "s.sh", None) == (0, 0, [], 0) + + +def test_line_without_a_number_is_skipped(mod: ModuleType, tmp_path: Path) -> None: + """A malformed entry must not abort the report.""" + out = tmp_path / "cov" / "proc" + out.mkdir(parents=True) + (out / "cov.xml").write_text( + _XML.format(name="s.sh", lines=''), + encoding="utf-8", + ) + covered, total, _, _ = mod._measure(tmp_path / "cov", "s.sh", None) + assert (covered, total) == (1, 1) + + +def test_find_source_prefers_the_trace_path(mod: ModuleType, tmp_path: Path) -> None: + """kcov records only a basename, so the trace is the source of the path.""" + source = tmp_path / "s.sh" + source.write_text("echo one\n", encoding="utf-8") + assert mod._find_source("s.sh", _trace(tmp_path, source, [1])) == source + + +def test_find_source_ignores_a_trace_path_that_is_gone( + mod: ModuleType, tmp_path: Path +) -> None: + """A traced path that no longer exists falls through to the repo search.""" + missing = tmp_path / "absent.sh" + trace = _trace(tmp_path, missing, [1]) + # Nothing in the repo is called absent.sh, so the fallback finds nothing. + assert mod._find_source("absent.sh", trace) is None + + +def test_find_source_without_a_trace_searches_the_repo(mod: ModuleType) -> None: + """The fallback resolves a uniquely-named file in the repo.""" + found = mod._find_source("shell_coverage_jail.sh", None) + assert found is not None + assert found.name == "shell_coverage_jail.sh" + + +def test_find_source_refuses_an_ambiguous_name(mod: ModuleType) -> None: + """Two files with one name yields no exclusion rather than a wrong guess.""" + assert mod._find_source("run_all.sh", None) is None + + +# -------------------------------------------------------------------------- +# main(): the gate itself +# -------------------------------------------------------------------------- + + +def _run_main( + mod: ModuleType, + monkeypatch: pytest.MonkeyPatch, + argv: list[str], +) -> None: + monkeypatch.setattr("sys.argv", ["shell_coverage_report.py", *argv]) + mod.main() + + +def test_main_prints_the_summary( + mod: ModuleType, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """A fully covered subject prints its percentage and passes the gate.""" + cov = _cov(tmp_path, "s.sh", {1: 1, 2: 1}) + _run_main(mod, monkeypatch, [str(cov), "s.sh", "100"]) + assert "s.sh: 2/2 lines = 100.00%" in capsys.readouterr().out + + +def test_main_lists_uncovered_lines( + mod: ModuleType, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """A gap is named, because the numbers alone do not help close it.""" + cov = _cov(tmp_path, "s.sh", {1: 1, 2: 0}) + with pytest.raises(SystemExit): + _run_main(mod, monkeypatch, [str(cov), "s.sh", "100"]) + assert "uncovered: 2" in capsys.readouterr().out + + +def test_main_notes_lines_outside_the_line_set( + mod: ModuleType, + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + capsys: pytest.CaptureFixture[str], +) -> None: + """A traced line kcov never listed is surfaced as a third-defect signal.""" + source = tmp_path / "s.sh" + source.write_text("echo one\necho two\n", encoding="utf-8") + cov = _cov(tmp_path, "s.sh", {1: 1}) + trace = _trace(tmp_path, source, [1, 2]) + _run_main(mod, monkeypatch, [str(cov), "s.sh", "100", str(trace)]) + assert "absent from kcov's line set" in capsys.readouterr().out + + +def test_main_exits_when_below_the_minimum( + mod: ModuleType, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Below the bar is a non-zero exit, which is what makes this a gate.""" + cov = _cov(tmp_path, "s.sh", {1: 1, 2: 0}) + with pytest.raises(SystemExit) as excinfo: + _run_main(mod, monkeypatch, [str(cov), "s.sh", "100"]) + assert "below the required" in str(excinfo.value) + + +def test_main_exits_when_nothing_was_instrumented( + mod: ModuleType, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Zero instrumented lines is an error, never a vacuous 100%.""" + cov = _cov(tmp_path, "other.sh", {1: 1}) + with pytest.raises(SystemExit) as excinfo: + _run_main(mod, monkeypatch, [str(cov), "s.sh", "100"]) + assert "instrumented no lines" in str(excinfo.value)