From d14e124d995b178f86579703bd453c06ad1ac3dc Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 20 Aug 2026 17:25:29 +0530 Subject: [PATCH 1/2] ci: fail the coverage gate when it verified nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Rust coverage lane reported success after compiling none of the changed code. Two independent fail-opens composed: a scoped libtest filter that matched nothing printed "running 0 tests ... ok" and exited 0, and diff-cover reported "No lines with coverage information in this diff" and also exited 0. Both read the absence of data as "nothing to check" rather than "we checked nothing". Add scripts/ci/assert-coverage-presence.sh, a hard gate asserting that every changed Rust source file which should have been compiled appears as an SF: record in the emitted lcov. It deliberately does not assert that tests ran or that changed lines are covered — diff-cover --fail-under=80 still owns the latter. Separating them is what keeps the gate free of false positives. Wire it into both modes of rust-coverage-changed.sh, escalate a zero-executed-tests scoped run to the full suite rather than failing it, and warn when diff-cover measures no lines at all. The exclusion set is measured, not guessed: replayed against the lcov-rust-core artifact of run 32108672413 it checks 1342 files and flags exactly the two uncompiled hosting sources, with no false positives. Closes #5613 --- .github/workflows/ci-lite.yml | 30 +- scripts/__tests__/coverage-presence.test.mjs | 271 +++++++++++++++++++ scripts/ci/assert-coverage-presence.sh | 220 +++++++++++++++ scripts/ci/coverage-presence-allowlist.txt | 28 ++ scripts/ci/rust-coverage-changed.sh | 54 +++- 5 files changed, 600 insertions(+), 3 deletions(-) create mode 100644 scripts/__tests__/coverage-presence.test.mjs create mode 100755 scripts/ci/assert-coverage-presence.sh create mode 100644 scripts/ci/coverage-presence-allowlist.txt diff --git a/.github/workflows/ci-lite.yml b/.github/workflows/ci-lite.yml index dd737809e7..5531969252 100644 --- a/.github/workflows/ci-lite.yml +++ b/.github/workflows/ci-lite.yml @@ -143,6 +143,8 @@ jobs: # The changed-files runner itself must trigger the lane that # runs it (it is also in rust-core-full → full-suite mode). - 'scripts/ci/rust-coverage-changed.sh' + - 'scripts/ci/assert-coverage-presence.sh' + - 'scripts/ci/coverage-presence-allowlist.txt' - 'Cargo.toml' - 'Cargo.lock' - 'build.rs' @@ -160,6 +162,8 @@ jobs: rust-core-full: - '.github/workflows/ci-lite.yml' - 'scripts/ci/rust-coverage-changed.sh' + - 'scripts/ci/assert-coverage-presence.sh' + - 'scripts/ci/coverage-presence-allowlist.txt' - 'Cargo.toml' - 'Cargo.lock' - 'build.rs' @@ -194,6 +198,8 @@ jobs: - 'scripts/mock-api/**' - 'scripts/*.mjs' - 'scripts/__tests__/**' + # Bash helpers with node --test coverage in scripts/__tests__/. + - 'scripts/ci/assert-coverage-presence.sh' # Test-wiring surface: a change here can orphan a test or add an # untested controller domain → run the inventory guard. inventory: @@ -1179,7 +1185,29 @@ jobs: --compare-branch="${{ steps.coverage-compare.outputs.ref }}" \ --fail-under=80 \ --html-report diff-coverage.html \ - --markdown-report diff-coverage.md + --markdown-report diff-coverage.md \ + --format json:diff-coverage.json + + # diff-cover exits 0 when it measured nothing at all ("No lines with + # coverage information in this diff"), which is how PR #5593 passed + # this gate with 1,643 uncompiled lines. Absence of data is not + # evidence of coverage — but it IS legitimate for a diff whose only + # Rust/TS changes are comments, imports or type declarations, so this + # only WARNS. The hard failure for "the file was never compiled" is + # scripts/ci/assert-coverage-presence.sh, which runs inside the core + # coverage lane and can tell those two cases apart. + # + # `total_num_lines` is diff-cover's own count of changed lines it + # found coverage rows for: 0 on the "no lines with coverage + # information" path, non-zero otherwise. Verified against + # diff-cover 10.5.1 (the version `>=9.2.0` resolves to today) on both + # an empty and a non-empty diff. `--format json:` rather than the + # `--json-report` alias, which that release deprecates. + measured="$(python3 -c 'import json; print(json.load(open("diff-coverage.json"))["total_num_lines"])')" + echo "diff-cover measured ${measured} changed line(s)" + if [ "${measured}" -eq 0 ]; then + echo "::warning::diff-cover measured 0 changed lines. If this PR changed executable code, that code was not compiled by any coverage lane — check the 'Rust Core Coverage' log for the coverage-presence gate." + fi - name: Upload diff-cover report if: always() && needs.changes.outputs.coverage == 'true' diff --git a/scripts/__tests__/coverage-presence.test.mjs b/scripts/__tests__/coverage-presence.test.mjs new file mode 100644 index 0000000000..c7304a25c2 --- /dev/null +++ b/scripts/__tests__/coverage-presence.test.mjs @@ -0,0 +1,271 @@ +// Unit tests for scripts/ci/assert-coverage-presence.sh — the hard gate that +// fails when the coverage lane produced no records at all for a changed Rust +// source file (#5613). +// +// Each test pins one clause of the script's `eligible()` filter. Delete the +// corresponding clause and exactly one test here goes red, which is what makes +// the exclusion list a reviewed ratchet rather than a pile of guesses. +// +// The fixtures are throwaway git repos: the script's `--all` mode reads +// `git ls-files`, and `eligible()` stats real paths. + +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.join( + path.dirname(fileURLToPath(import.meta.url)), + "..", + "..", +); +const script = path.join( + repoRoot, + "scripts", + "ci", + "assert-coverage-presence.sh", +); + +/** A source file with a real `fn`, so it is instrumentable. */ +const WITH_FN = "pub fn thing() -> u8 {\n 1\n}\n"; +/** A barrel module: re-exports only, no `fn`, can never produce a region. */ +const NO_FN = "pub mod a;\npub use a::Thing;\n"; + +/** + * Build a throwaway repo, write `files`, and run the gate over it. + * + * @param {Record} files repo-relative path -> contents + * @param {string[]} coveredPaths paths to emit as `SF:` records + * @param {string[]} args args after the lcov path + * @param {string} allowlist contents of the allowlist file, if any + */ +function run(files, coveredPaths, args, allowlist = null) { + const cwd = fs.mkdtempSync(path.join(os.tmpdir(), "openhuman-cov-presence-")); + for (const [rel, body] of Object.entries(files)) { + const abs = path.join(cwd, rel); + fs.mkdirSync(path.dirname(abs), { recursive: true }); + fs.writeFileSync(abs, body); + } + execFileSync("git", ["init", "-q", "."], { cwd }); + execFileSync("git", ["add", "-A"], { cwd }); + + const lcov = path.join(cwd, "cov.info"); + fs.writeFileSync( + lcov, + coveredPaths.map((p) => `SF:${cwd}/${p}\nDA:1,1\nend_of_record\n`).join(""), + ); + + const env = { ...process.env }; + if (allowlist !== null) { + const listPath = path.join(cwd, "allow.txt"); + fs.writeFileSync(listPath, allowlist); + env.ALLOWLIST = listPath; + } else { + // Default to no allowlist so a fixture never picks up the repo's real one. + env.ALLOWLIST = path.join(cwd, "absent.txt"); + } + + try { + const stdout = execFileSync("bash", [script, lcov, ...args], { + cwd, + encoding: "utf8", + env, + }); + return { status: 0, output: stdout }; + } catch (err) { + return { + status: err.status, + output: `${err.stdout ?? ""}${err.stderr ?? ""}`, + }; + } +} + +test("fails and names a changed file that produced no coverage records", () => { + const res = run( + { "src/a/covered.rs": WITH_FN, "src/a/uncompiled.rs": WITH_FN }, + ["src/a/covered.rs"], + ["--files", "src/a/covered.rs", "src/a/uncompiled.rs"], + ); + assert.equal(res.status, 1); + assert.match( + res.output, + /src\/a\/uncompiled\.rs produced no coverage records/, + ); + assert.doesNotMatch(res.output, /file=src\/a\/covered\.rs/); +}); + +test("passes when every eligible changed file is present in the lcov", () => { + const res = run( + { "src/a/covered.rs": WITH_FN }, + ["src/a/covered.rs"], + ["--files", "src/a/covered.rs"], + ); + assert.equal(res.status, 0); + assert.match(res.output, /clean/); +}); + +test("skips barrel modules that declare no fn", () => { + const res = run({ "src/a/mod.rs": NO_FN }, [], ["--files", "src/a/mod.rs"]); + assert.equal(res.status, 0); + assert.match(res.output, /checked 0 eligible/); +}); + +test("a fn mentioned only inside a line comment does not make a file eligible", () => { + const res = run( + { "src/a/mod.rs": "// pub fn documented() {}\npub use x::Y;\n" }, + [], + ["--files", "src/a/mod.rs"], + ); + assert.equal(res.status, 0); + assert.match(res.output, /checked 0 eligible/); +}); + +test("large files stay eligible (regression: pipefail + grep -q SIGPIPE)", () => { + // The first implementation used `grep -v … | grep -q …`. Under `set -o + // pipefail` the reader exits at the first match, the writer dies of SIGPIPE, + // and the pipeline returns 141 — so any file long enough to still be + // streaming read as "no fn" and was silently skipped. That excluded 299 of + // 1,377 eligible sources, including the 937-line file this gate was written + // to catch. The `fn` here is deliberately at the top with bulk after it. + const big = WITH_FN + "// filler\n".repeat(20000); + const res = run({ "src/a/big.rs": big }, [], ["--files", "src/a/big.rs"]); + assert.equal( + res.status, + 1, + "a large uncovered source file must still be checked", + ); + assert.match(res.output, /checked 1 eligible/); + assert.match(res.output, /src\/a\/big\.rs produced no coverage records/); +}); + +test("skips test sources, stub.rs, per-OS modules and deleted paths", () => { + const res = run( + { + "src/a/thing_tests.rs": WITH_FN, + "src/a/thing_test.rs": WITH_FN, + "src/a/tests.rs": WITH_FN, + "src/a/test_support.rs": WITH_FN, + "src/a/tests/helper.rs": WITH_FN, + "src/a/stub.rs": WITH_FN, + "src/a/macos.rs": WITH_FN, + "src/a/windows.rs": WITH_FN, + }, + [], + [ + "--files", + "src/a/thing_tests.rs", + "src/a/thing_test.rs", + "src/a/tests.rs", + "src/a/test_support.rs", + "src/a/tests/helper.rs", + "src/a/stub.rs", + "src/a/macos.rs", + "src/a/windows.rs", + "src/a/deleted.rs", + ], + ); + assert.equal(res.status, 0); + assert.match(res.output, /checked 0 eligible/); +}); + +test("skips non-Rust paths, crate roots and src/bin", () => { + const res = run( + { + "src/lib.rs": WITH_FN, + "src/main.rs": WITH_FN, + "src/bin/tool.rs": WITH_FN, + "src/a/README.md": "# doc\n", + }, + [], + [ + "--files", + "src/lib.rs", + "src/main.rs", + "src/bin/tool.rs", + "src/a/README.md", + ], + ); + assert.equal(res.status, 0); + assert.match(res.output, /checked 0 eligible/); +}); + +test("skips families that are uncovered by design", () => { + const res = run( + { + "src/tui/app.rs": WITH_FN, + "src/openhuman/test_support/reset.rs": WITH_FN, + "src/openhuman/tools/impl/browser/native_backend.rs": WITH_FN, + }, + [], + [ + "--files", + "src/tui/app.rs", + "src/openhuman/test_support/reset.rs", + "src/openhuman/tools/impl/browser/native_backend.rs", + ], + ); + assert.equal(res.status, 0); + assert.match(res.output, /checked 0 eligible/); +}); + +test("normalises .. inside SF: paths so #[path] modules are not false-failed", () => { + // rustc records the literal string from a `#[path = "../x.rs"]` attribute, + // so the lcov can name a real, compiled file by a non-canonical path. + const res = run( + { "src/a/b.rs": WITH_FN }, + ["src/a/sub/../b.rs"], + ["--files", "src/a/b.rs"], + ); + assert.equal(res.status, 0); + assert.match(res.output, /clean/); +}); + +test("honours the allowlist, ignoring comments and blank lines", () => { + const files = { "src/a/gated.rs": WITH_FN }; + const bare = run(files, [], ["--files", "src/a/gated.rs"]); + assert.equal(bare.status, 1, "sanity: unlisted the file must fail"); + + const listed = run( + files, + [], + ["--files", "src/a/gated.rs"], + "# a reason\n\nsrc/a/gated.rs\n", + ); + assert.equal(listed.status, 0); + assert.match(listed.output, /checked 0 eligible/); +}); + +test("--all walks the tracked tree", () => { + const res = run( + { "src/a/covered.rs": WITH_FN, "src/a/uncompiled.rs": WITH_FN }, + ["src/a/covered.rs"], + ["--all"], + ); + assert.equal(res.status, 1); + assert.match(res.output, /checked 2 eligible/); + assert.match( + res.output, + /src\/a\/uncompiled\.rs produced no coverage records/, + ); +}); + +test("exits 2 on a missing lcov file and on bad usage", () => { + const cwd = fs.mkdtempSync( + path.join(os.tmpdir(), "openhuman-cov-presence-usage-"), + ); + const bad = (args) => { + try { + execFileSync("bash", [script, ...args], { cwd, encoding: "utf8" }); + return 0; + } catch (err) { + return err.status; + } + }; + assert.equal(bad([path.join(cwd, "nope.info"), "--all"]), 2); + fs.writeFileSync(path.join(cwd, "cov.info"), ""); + assert.equal(bad([path.join(cwd, "cov.info"), "--bogus"]), 2); + assert.equal(bad([path.join(cwd, "cov.info")]), 2); +}); diff --git a/scripts/ci/assert-coverage-presence.sh b/scripts/ci/assert-coverage-presence.sh new file mode 100755 index 0000000000..d9c9eef993 --- /dev/null +++ b/scripts/ci/assert-coverage-presence.sh @@ -0,0 +1,220 @@ +#!/usr/bin/env bash +# Coverage-presence gate: fail when a changed Rust source file produced NO +# coverage records at all — i.e. the lane never compiled it, so neither the +# scoped test run nor diff-cover could possibly have verified it. +# +# WHY THIS EXISTS (PR #5593). `src/openhuman/hosting/**` is gated behind a Cargo +# feature that is in neither `[features] default` nor +# `scripts/ci/product-features.txt`, so the coverage lane compiled none of it. +# The scoped libtest filter matched nothing (`running 0 tests … ok`) and +# diff-cover reported "No lines with coverage information in this diff". Both +# read the ABSENCE of data as "nothing to check" rather than "we checked +# nothing", and 1,643 lines — including a 511-line test file — merged green. +# +# WHAT THIS ASSERTS, and deliberately not more: every changed Rust source file +# that should have been compiled appears as an `SF:` record in the lcov. It says +# nothing about how well those lines are covered — `diff-cover --fail-under=80` +# still owns that. Separating the two is what keeps this gate free of false +# positives: "no rows at all" is a build-configuration fact, whereas "too few +# covered lines" is a judgement call that already has an owner. +# +# Usage: +# assert-coverage-presence.sh --files ... # scoped mode +# assert-coverage-presence.sh --all # whole-tree mode +# +# Exit: 0 clean · 1 unverified files found · 2 usage/environment error. +set -euo pipefail + +ALLOWLIST="${ALLOWLIST:-scripts/ci/coverage-presence-allowlist.txt}" + +log() { echo "[ci][cov-presence] $*"; } +die() { + echo "[ci][cov-presence] $*" >&2 + exit 2 +} + +# Source paths whose absence from the lcov is EXPECTED and correct. Each entry +# is a path prefix plus the reason it can never appear, so a future reader can +# tell "excluded on purpose" from "forgotten" — the ambiguity that let #4918 sit. +# +# src/tui/ `tui` is default-OFF and deliberately never +# forwarded (INTENTIONALLY_NOT_FORWARDED in +# scripts/lib/feature-forwarding.mjs). +# src/openhuman/test_support/ `e2e-test-support`; the destructive +# `openhuman.test_reset` RPC must never ship. +# .../browser/native_backend.rs `browser-native`, an opt-in dev backend. +UNCOVERED_BY_DESIGN='^(src/tui/|src/openhuman/test_support/|src/openhuman/tools/impl/browser/native_backend\.rs$)' + +usage() { + cat <<'USAGE' +Usage: assert-coverage-presence.sh (--all | --files ...) + --all check every eligible src/**/*.rs in the tree + --files … check only the given paths (repo-relative) +USAGE +} + +[ "$#" -ge 2 ] || { + usage >&2 + exit 2 +} +LCOV="$1" +shift +[ -f "${LCOV}" ] || die "lcov file not found: ${LCOV}" + +MODE="" +declare -a want=() +case "${1:-}" in + --all) + MODE=all + shift + ;; + --files) + MODE=files + shift + while [ "$#" -gt 0 ]; do + want+=("$1") + shift + done + ;; + -h | --help) + usage + exit 0 + ;; + *) + usage >&2 + exit 2 + ;; +esac + +# ---- the set of files the coverage build actually produced records for ------- +# +# `SF:` paths are absolute in CI (/__w/openhuman/openhuman/src/...). Two of them +# also contain `..`, because rustc records the literal path written in a +# `#[path = "../foo.rs"]` attribute rather than a canonical one — today +# `config/schema/load/../load_user_state.rs` and +# `tools/../integrations/test_support.rs`. Both are real, compiled, measured +# files; without normalisation they would be reported unverified. Normalise +# before comparing. +# +# Two forms are recorded for every path, and a file matches either. +# +# 1. the `${PWD}`-relative form, which is what CI produces directly; and +# 2. everything from the last `/src/` onward, which is independent of where +# the checkout lives. +# +# (2) exists because the recorded prefix and `${PWD}` are not reliably the same +# string. Bash resolves its working directory physically, so a checkout reached +# through a symlink — macOS `$TMPDIR` under `/var -> private/var`, a +# bind-mounted or symlinked CI workspace — records `/var/…` while `${PWD}` says +# `/private/var/…`. Prefix stripping then removes nothing, every path stays +# absolute, nothing matches, and the gate false-fails the ENTIRE diff. That is +# the worst failure this script has, so it does not depend on the prefix. +# +# The last `/src/` rather than the first: a developer checkout at +# `~/src/openhuman/` contains two, and the repo-relative path is the trailing +# one. Unambiguous here because no tracked path under `src/` contains a nested +# `src/` component, and all 1,354 `SF:` records in the reference artifact carry +# the `/src/` marker. +covered_file="$(mktemp)" +trap 'rm -f "${covered_file}"' EXIT +sed -n 's/^SF://p' "${LCOV}" \ + | sed "s#^${PWD}/##" \ + | python3 -c 'import sys, posixpath +for line in sys.stdin: + line = line.strip() + if not line: + continue + path = posixpath.normpath(line) + print(path) + marker = path.rfind("/src/") + if marker != -1: + print(path[marker + 1 :])' \ + | sort -u >"${covered_file}" + +covered() { grep -Fxq "$1" "${covered_file}"; } + +allowlisted() { + [ -f "${ALLOWLIST}" ] || return 1 + grep -v '^[[:space:]]*#' "${ALLOWLIST}" 2>/dev/null \ + | grep -v '^[[:space:]]*$' \ + | grep -Fxq "$1" +} + +# Does this file declare a function outside a line comment? +# +# ONE awk process, deliberately not `grep -v … | grep -q …`. Under the script's +# `set -o pipefail`, `grep -q` exits at the first match, the upstream `grep -v` +# dies of SIGPIPE, and the pipeline reports 141 — so the file reads as "no fn" +# and is silently skipped. It only bites files long enough for the writer to +# still be going when the reader leaves, i.e. exactly the large files this gate +# most needs to check: it wrongly excluded 299 of 1,377 eligible sources, +# `src/openhuman/hosting/tools.rs` (937 lines) among them. +# +# The pattern avoids `\b` (a GNU extension) so the check behaves identically +# under the BSD grep/awk a contributor runs locally and the GNU one in CI. +has_fn() { + awk ' + /^[[:space:]]*\/\// { next } + /(^|[^A-Za-z0-9_])fn[[:space:]]+[A-Za-z_]/ { found = 1; exit } + END { exit(found ? 0 : 1) } + ' "$1" +} + +# ---- eligibility ------------------------------------------------------------- +# +# A path is CHECKED only when every one of these holds. Each exclusion is a +# category for which "no lcov rows" is the correct, expected outcome; a rule +# without them false-fails on 623 of 1,972 files (measured against the +# lcov-rust-core artifact of run 32108672413). +eligible() { + local f="$1" base + base="$(basename "${f}")" + + case "${f}" in *.rs) ;; *) return 1 ;; esac # non-Rust: assets, .md, fixtures + case "${f}" in src/*) ;; *) return 1 ;; esac # only crate sources + [ -f "${f}" ] || return 1 # deleted / renamed-away + case "${f}" in src/lib.rs | src/main.rs | src/bin/*) return 1 ;; esac + # Test-only sources. We do not demand coverage OF test code, and a test file + # only ever appears in the lcov as a side effect of its own execution. + case "${base}" in *_tests.rs | *_test.rs | tests.rs | test.rs | test_support.rs) return 1 ;; esac + case "${f}" in */tests/* | */test/*) return 1 ;; esac + # Facade stubs compile only in the OFF direction of their gate; under the + # product feature set the real module compiles instead. 13 of these exist. + [ "${base}" = "stub.rs" ] && return 1 + # Per-OS modules behind #[cfg(target_os)]; CI is Linux. + case "${base}" in macos.rs | windows.rs) return 1 ;; esac + echo "${f}" | grep -Eq "${UNCOVERED_BY_DESIGN}" && return 1 + allowlisted "${f}" && return 1 + # No instrumentable code: barrel `mod.rs`, pure type/const modules. 319 files + # have no `fn` at all and can never produce a coverage region. + has_fn "${f}" || return 1 + return 0 +} + +declare -a candidates=() +if [ "${MODE}" = all ]; then + while IFS= read -r f; do candidates+=("${f}"); done < <(git ls-files 'src/*.rs' 'src/**/*.rs' | sort -u) +else + candidates=("${want[@]+"${want[@]}"}") +fi + +declare -a unverified=() +checked=0 +for f in "${candidates[@]+"${candidates[@]}"}"; do + eligible "${f}" || continue + checked=$((checked + 1)) + covered "${f}" || unverified+=("${f}") +done + +log "checked ${checked} eligible source file(s) against $(wc -l <"${covered_file}" | tr -d ' ') covered path(s)" + +if [ "${#unverified[@]}" -eq 0 ]; then + log "clean — every eligible changed source file produced coverage records" + exit 0 +fi + +echo "::error::Coverage lane produced NO records for ${#unverified[@]} changed source file(s) — they were never compiled, so nothing verified them." +for f in "${unverified[@]}"; do + echo "::error file=${f}::${f} produced no coverage records. The coverage lane compiles 'default + scripts/ci/product-features.txt'; if this file sits behind a Cargo feature in neither list it was never built. Fix by adding the gate to product-features.txt (and the shell forwarding list), or record it in scripts/ci/coverage-presence-allowlist.txt with a reason." +done +exit 1 diff --git a/scripts/ci/coverage-presence-allowlist.txt b/scripts/ci/coverage-presence-allowlist.txt new file mode 100644 index 0000000000..f6d42734ce --- /dev/null +++ b/scripts/ci/coverage-presence-allowlist.txt @@ -0,0 +1,28 @@ +# Source files the coverage lane legitimately cannot produce records for, one +# repo-relative path per line. `#` comments and blank lines ignored. +# +# This is NOT a way to silence the gate. Every entry is a statement that the +# file is deliberately absent from the product build, and the reason must say +# which gate excludes it and why that is intended. Structural categories +# (test sources, stub.rs, per-OS modules, src/tui/, test_support/) are handled +# by the script itself and must NOT be listed here. +# +# The list is a ratchet: entries come off, they do not go on. Adding one needs a +# written justification in the PR body and a reviewer who reads it. + +# TEMPORARY — remove both lines in #5619. +# +# The `hosting` Cargo gate (Cargo.toml:756, `hosting = ["dep:tinyhosts"]`) is in +# neither `[features] default` nor `scripts/ci/product-features.txt`, so +# `src/openhuman/hosting/**` is #[cfg]'d out at src/openhuman/mod.rs:29-30 and +# the coverage lane never compiles it. That is the exact defect #5613 describes +# and this gate exists to catch — it is listed here only because the gate lands +# before the classification fix, not because the absence is correct. +# +# #5619 ("fix(hosting): compile the hosting family into the product") adds the +# gate to the product set. When it merges, delete these two lines; the gate then +# verifies hosting like any other family. If #5619 is closed without landing, +# these lines must be replaced by a written decision that hosting ships +# uncompiled, not left here by default. +src/openhuman/hosting/mod.rs +src/openhuman/hosting/tools.rs diff --git a/scripts/ci/rust-coverage-changed.sh b/scripts/ci/rust-coverage-changed.sh index 8f855a9a83..e7ff50fc46 100755 --- a/scripts/ci/rust-coverage-changed.sh +++ b/scripts/ci/rust-coverage-changed.sh @@ -17,6 +17,16 @@ # no longer counts on the fast lane. The full suite still runs on main→release # PRs (Release CI). # +# TWO GATES GUARD THE "WE VERIFIED NOTHING" CASE (PR #5593): +# 1. scripts/ci/assert-coverage-presence.sh — hard failure when a changed +# source file produced no lcov records at all, i.e. the lane never +# compiled it. This is the precise one; it names the files. +# 2. A zero-executed-tests scoped run ESCALATES to the full suite rather than +# failing. Scoping that selects no tests is unsafe scoping, and this +# script's standing policy for unsafe scoping is to widen, not to redden — +# a domain that legitimately owns no unit tests (there are five today, +# e.g. core::shutdown) must not turn every PR touching it red. +# # Inputs (env): # FULL "true" → run the full suite (build-config / lib.rs / script # changes, detected by paths-filter) @@ -63,6 +73,26 @@ llvm_cov() { bash scripts/ci-cancel-aware.sh cargo llvm-cov --features "${PRODUCT_FEATURES}" "$@" } +# Total libtest cases executed across every scoped/full run in this invocation. +# `run_counted` tees libtest output so the count can be read without changing +# what the log looks like. `${PIPESTATUS[0]}` — not `$?` — carries the cargo +# exit status through the pipe; reading `$?` here would report tee's status and +# turn a failing suite green. +TESTS_RUN=0 + +run_counted() { + local log rc n + log="$(mktemp)" + set +e + "$@" 2>&1 | tee "${log}" + rc=${PIPESTATUS[0]} + set -e + n="$(sed -n 's/^running \([0-9]\{1,\}\) tests\{0,1\}$/\1/p' "${log}" | awk '{s+=$1} END {print s+0}')" + TESTS_RUN=$((TESTS_RUN + n)) + rm -f "${log}" + return "${rc}" +} + integration_test_targets() { find tests -maxdepth 1 -type f -name '*.rs' -print | sed -e 's#^tests/##' -e 's#\.rs$##' | @@ -133,6 +163,12 @@ run_full() { done < <(integration_test_targets) log "merging coverage into ${OUT}" llvm_cov report --lcov --output-path "${OUT}" + # FULL mode has no changed-file list (the workflow blanks CHANGED_FILES to + # stay under the container's argv limit), so assert the whole-tree invariant + # instead: no eligible source file may be missing from a full product build's + # coverage. This is the mode PR #5578 ran in when it first landed the + # uncompiled hosting family, and it is the mode that would have caught it. + bash scripts/ci/assert-coverage-presence.sh "${OUT}" --all exit 0 } @@ -280,15 +316,29 @@ llvm_cov clean --workspace if [ "${#lib_filters[@]}" -gt 0 ]; then log "running scoped lib unit tests with filters: ${lib_filters[*]}" # libtest ORs multiple positional filters — one run covers all domains. - llvm_cov --no-report --no-fail-fast -p openhuman --lib -- "${lib_filters[@]}" + run_counted llvm_cov --no-report --no-fail-fast -p openhuman --lib -- "${lib_filters[@]}" fi if [ "${#test_targets[@]}" -gt 0 ]; then for t in "${test_targets[@]}"; do log "running changed integration-test target: ${t}" - run_integration_target "${t}" + run_counted run_integration_target "${t}" done fi log "merging coverage into ${OUT}" llvm_cov report --lcov --output-path "${OUT}" + +# Gate 1 (precise, hard): did the lane produce ANY coverage records for the +# files this PR changed? Run before the zero-test escalation so the hosting-class +# defect — a file the build never compiled — fails in ~10 minutes with the file +# names, instead of first spending ~40 minutes on a full suite that cannot +# compile it either. +bash scripts/ci/assert-coverage-presence.sh "${OUT}" --files "${files[@]}" + +# Gate 2 (imprecise, safe): a scoped run that executed no tests verified +# nothing. Widen rather than fail — see the header note. +if [ "${TESTS_RUN}" -eq 0 ]; then + log "scoped run executed 0 tests (filters: ${lib_filters[*]-none}; targets: ${test_targets[*]-none})" + run_full "scoped run executed 0 tests — scoping selected no coverage" +fi From 2748a157f618b4cc5f2ca4bc34ab55ea16d48101 Mon Sep 17 00:00:00 2001 From: M3gA-Mind Date: Thu, 20 Aug 2026 17:41:14 +0530 Subject: [PATCH 2/2] ci: preserve a failed raw coverage module through run_counted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `run_integration_target raw_coverage_all` loops one llvm_cov call per module. The loop's exit status is its last iteration's, so a module that fails followed by one that succeeds reported success. Ambient errexit used to mask this: the function was called bare, so a failing llvm_cov aborted the script. Wrapping it in `run_counted` removed that — the command runs inside a pipeline under `set +e`, which disables errexit for everything underneath — and turned a latent hazard into a live one that would let a red suite go green. Return on the first failing module. That restores the previous fail-fast timing exactly (no module ran after a failure before, and none does now) and states the guarantee in the code instead of relying on a shell option set elsewhere. Add scripts/__tests__/coverage-runner-status.test.mjs, which evaluates the real function bodies extracted from the runner rather than a copy of them, so the test cannot keep passing after the original regresses. Verified non-vacuous: reverting the `|| return` reddens exactly the failed-module test. Also add per-function comments to assert-coverage-presence.sh. --- .../__tests__/coverage-runner-status.test.mjs | 143 ++++++++++++++++++ scripts/ci/assert-coverage-presence.sh | 11 ++ scripts/ci/rust-coverage-changed.sh | 13 +- 3 files changed, 166 insertions(+), 1 deletion(-) create mode 100644 scripts/__tests__/coverage-runner-status.test.mjs diff --git a/scripts/__tests__/coverage-runner-status.test.mjs b/scripts/__tests__/coverage-runner-status.test.mjs new file mode 100644 index 0000000000..9e7ae367f4 --- /dev/null +++ b/scripts/__tests__/coverage-runner-status.test.mjs @@ -0,0 +1,143 @@ +// Regression tests for status propagation in scripts/ci/rust-coverage-changed.sh. +// +// `run_counted` runs its command inside a pipeline under `set +e` so it can tee +// and count libtest output. That disables errexit for everything underneath, +// which is exactly the condition under which a loop silently swallows a failed +// iteration. These tests pin that a failing coverage module still fails the job. +// +// They evaluate the REAL function bodies, extracted from the script at test +// time, rather than a transcription of them — a copy would keep passing after +// the original regressed, which is the whole failure mode being guarded. + +import assert from "node:assert/strict"; +import { execFileSync } from "node:child_process"; +import fs from "node:fs"; +import path from "node:path"; +import test from "node:test"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.join( + path.dirname(fileURLToPath(import.meta.url)), + "..", + "..", +); +const runner = path.join(repoRoot, "scripts", "ci", "rust-coverage-changed.sh"); + +/** Extract the runner's `TESTS_RUN` accumulator declaration, verbatim. */ +function extractTestsRunDecl() { + const source = fs.readFileSync(runner, "utf8"); + const decl = source.split("\n").find((line) => /^TESTS_RUN=/.test(line)); + assert.ok( + decl, + `TESTS_RUN= not found in ${runner} — did the accumulator get renamed?`, + ); + return decl; +} + +/** Extract a top-level `name() { … }` block from the runner, verbatim. */ +function extractFunction(name) { + const source = fs.readFileSync(runner, "utf8"); + const start = source.indexOf(`${name}() {`); + assert.notEqual( + start, + -1, + `${name}() not found in ${runner} — did it get renamed?`, + ); + const end = source.indexOf("\n}\n", start); + assert.notEqual(end, -1, `could not find the end of ${name}()`); + return source.slice(start, end + 3); +} + +/** + * Run a bash snippet with the named runner functions spliced in and the heavy + * dependencies stubbed. + * + * @param {string[]} functions names to lift out of the runner + * @param {string} preamble stubs, defined before the real functions + * @param {string} body the assertion driver + */ +function withRunnerFunctions(functions, preamble, body) { + const script = [ + "set -euo pipefail", + extractTestsRunDecl(), + preamble, + ...functions.map(extractFunction), + body, + ].join("\n"); + try { + return { + status: 0, + output: execFileSync("bash", ["-c", script], { encoding: "utf8" }), + }; + } catch (err) { + return { + status: err.status, + output: `${err.stdout ?? ""}${err.stderr ?? ""}`, + }; + } +} + +test("a failed raw coverage module fails the run even when a later module succeeds", () => { + // The exact shape CodeRabbit flagged: module `first` fails, `second` passes. + // The loop's status is its last iteration's, so without an explicit `return` + // the failure is discarded and CI goes green on a red suite. + const res = withRunnerFunctions( + ["run_counted", "run_integration_target"], + [ + "log() { printf '%s\\n' \"$*\"; }", + "raw_coverage_modules() { printf 'first\\nsecond\\n'; }", + // Fails for `first::`, succeeds otherwise. + "llvm_cov() { for a in \"$@\"; do case \"$a\" in first::) echo 'module first FAILED'; return 7 ;; esac; done; echo 'module ok'; return 0; }", + ].join("\n"), + [ + "if run_counted run_integration_target raw_coverage_all; then", + " echo 'WRAPPER-SAID-SUCCESS'; exit 0", + "else", + ' echo "WRAPPER-SAID-FAILURE rc=$?"; exit 3', + "fi", + ].join("\n"), + ); + + assert.equal( + res.status, + 3, + `expected the wrapper to report failure, got:\n${res.output}`, + ); + assert.match(res.output, /WRAPPER-SAID-FAILURE/); + // Fail-fast: nothing after the failing module may run. + assert.doesNotMatch(res.output, /running raw coverage module: second/); +}); + +test("run_counted propagates the command status, not tee's", () => { + // `${PIPESTATUS[0]}` rather than `$?`. Reading `$?` after the pipe reports + // tee, which always succeeds, turning every failing suite green. + const res = withRunnerFunctions( + ["run_counted"], + "boom() { echo 'running 3 tests'; return 9; }", + "run_counted boom || { echo \"rc=$?\"; exit 0; }; echo 'NO-FAILURE-SEEN'; exit 1", + ); + assert.equal(res.status, 0, res.output); + assert.match(res.output, /rc=9/); +}); + +test("run_counted sums libtest counts across calls and passes success through", () => { + const res = withRunnerFunctions( + ["run_counted"], + "some() { echo 'running 12 tests'; }\nmore() { echo 'running 1 test'; }\nnone() { echo 'running 0 tests'; }", + 'run_counted some; run_counted more; run_counted none; echo "TOTAL=${TESTS_RUN}"', + ); + assert.equal(res.status, 0, res.output); + // 12 + 1 + 0 — and "1 test" singular must parse, or a one-test domain looks + // like a zero-test run and needlessly escalates to the full suite. + assert.match(res.output, /TOTAL=13/); +}); + +test("run_counted counts zero for a run that executed no tests", () => { + const res = withRunnerFunctions( + ["run_counted"], + "nothing() { echo 'running 0 tests'; echo 'test result: ok. 0 passed; 12202 filtered out'; }", + 'run_counted nothing; [ "${TESTS_RUN}" -eq 0 ] && echo \'ZERO\' || echo "NONZERO=${TESTS_RUN}"', + ); + assert.equal(res.status, 0, res.output); + assert.match(res.output, /ZERO/); +}); diff --git a/scripts/ci/assert-coverage-presence.sh b/scripts/ci/assert-coverage-presence.sh index d9c9eef993..729219e20a 100755 --- a/scripts/ci/assert-coverage-presence.sh +++ b/scripts/ci/assert-coverage-presence.sh @@ -27,7 +27,12 @@ set -euo pipefail ALLOWLIST="${ALLOWLIST:-scripts/ci/coverage-presence-allowlist.txt}" +# Progress line on stdout. Prefixed so the lane's log stays greppable. log() { echo "[ci][cov-presence] $*"; } + +# Usage/environment error: stderr, exit 2. Distinct from exit 1 ("found +# unverified files") so a caller can tell a broken invocation from a real +# finding. die() { echo "[ci][cov-presence] $*" >&2 exit 2 @@ -45,6 +50,7 @@ die() { # .../browser/native_backend.rs `browser-native`, an opt-in dev backend. UNCOVERED_BY_DESIGN='^(src/tui/|src/openhuman/test_support/|src/openhuman/tools/impl/browser/native_backend\.rs$)' +# Invocation help, printed to stdout for --help and to stderr on a usage error. usage() { cat <<'USAGE' Usage: assert-coverage-presence.sh (--all | --files ...) @@ -131,8 +137,13 @@ for line in sys.stdin: print(path[marker + 1 :])' \ | sort -u >"${covered_file}" +# Did the coverage build emit records for this repo-relative path? +# Fixed-string, whole-line: a path containing regex metacharacters cannot match +# the wrong entry. covered() { grep -Fxq "$1" "${covered_file}"; } +# Is this path recorded in the allowlist as deliberately uncompiled? +# Absent allowlist means nothing is exempt, which is the safe direction. allowlisted() { [ -f "${ALLOWLIST}" ] || return 1 grep -v '^[[:space:]]*#' "${ALLOWLIST}" 2>/dev/null \ diff --git a/scripts/ci/rust-coverage-changed.sh b/scripts/ci/rust-coverage-changed.sh index e7ff50fc46..803551baa7 100755 --- a/scripts/ci/rust-coverage-changed.sh +++ b/scripts/ci/rust-coverage-changed.sh @@ -135,10 +135,21 @@ run_integration_target() { # globals (env vars, event bus handlers, auth tokens, singleton stores). # Run one process per generated module filter to preserve the former # per-binary isolation contract while still paying only one link. + # + # `|| return` is load-bearing, not defensive noise. This loop's exit status + # is that of its LAST iteration, so a module that fails followed by one that + # succeeds reports success. That used to be masked by ambient errexit — the + # function was called bare, so a failing `llvm_cov` aborted the script here. + # It is no longer: `run_counted` runs its command inside a pipeline with + # `set +e`, which disables errexit for everything underneath, so without this + # the failure is silently discarded and a red suite goes green. + # + # Returning on the first failure also preserves the previous fail-fast + # timing exactly: no module ran after a failure before, and none does now. while IFS= read -r module; do [ -n "${module}" ] || continue log "running raw coverage module: ${module}" - llvm_cov --no-report --no-fail-fast -p openhuman --test "${target}" -- "${module}::" --test-threads=1 + llvm_cov --no-report --no-fail-fast -p openhuman --test "${target}" -- "${module}::" --test-threads=1 || return done < <(raw_coverage_modules) elif [ "${target}" = "json_rpc_e2e" ]; then # This target exercises process-global runtime/config state. Its tests take