diff --git a/.github/workflows/ci-lite.yml b/.github/workflows/ci-lite.yml index 3dc9116e8d..b0b9bbc18a 100644 --- a/.github/workflows/ci-lite.yml +++ b/.github/workflows/ci-lite.yml @@ -151,6 +151,10 @@ jobs: # entry, resolved to `["rust-tauri"]` alone, skipping every lane # that would have compiled the family it enabled. - 'scripts/ci/product-features.*' + # The coverage-presence gate and its allowlist decide whether this + # lane FAILS, so they arm it for the same reason. + - 'scripts/ci/assert-coverage-presence.sh' + - 'scripts/ci/coverage-presence-allowlist.txt' - 'Cargo.toml' - 'Cargo.lock' - 'build.rs' @@ -180,6 +184,8 @@ jobs: # changes what compiles, which invalidates per-module scoping # exactly as a `Cargo.toml` change does. - 'scripts/ci/product-features.*' + - 'scripts/ci/assert-coverage-presence.sh' + - 'scripts/ci/coverage-presence-allowlist.txt' - 'Cargo.toml' - 'Cargo.lock' - 'build.rs' @@ -222,6 +228,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: @@ -1207,7 +1215,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..b380e18204 --- /dev/null +++ b/scripts/__tests__/coverage-presence.test.mjs @@ -0,0 +1,360 @@ +// 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("--all still enumerates when git ls-files fails (CI dubious-ownership)", () => { + // Regression for run 32367545922: actions/checkout registers `safe.directory` + // under a temporarily overridden HOME, so `git ls-files` in a later step dies + // with `fatal: detected dubious ownership`. Read through a process + // substitution that yielded an EMPTY candidate list, and the gate reported + // "clean" having checked ZERO files — the verified-nothing fail-open this + // script exists to close, reproduced inside it. + const cwd = fs.mkdtempSync( + path.join(os.tmpdir(), "openhuman-cov-presence-nogit-"), + ); + fs.mkdirSync(path.join(cwd, "src", "a"), { recursive: true }); + fs.writeFileSync(path.join(cwd, "src", "a", "uncompiled.rs"), WITH_FN); + fs.writeFileSync(path.join(cwd, "cov.info"), ""); + + // A `git` on PATH that always fails, standing in for dubious ownership. + const bin = fs.mkdtempSync(path.join(os.tmpdir(), "openhuman-fakegit-")); + fs.writeFileSync( + path.join(bin, "git"), + "#!/usr/bin/env bash\necho 'fatal: detected dubious ownership' >&2\nexit 128\n", + ); + fs.chmodSync(path.join(bin, "git"), 0o755); + + let status = 0; + let output = ""; + try { + output = execFileSync( + "bash", + [script, path.join(cwd, "cov.info"), "--all"], + { + cwd, + encoding: "utf8", + env: { + ...process.env, + PATH: `${bin}:${process.env.PATH}`, + ALLOWLIST: path.join(cwd, "absent.txt"), + }, + }, + ); + } catch (err) { + status = err.status; + output = `${err.stdout ?? ""}${err.stderr ?? ""}`; + } + + assert.match(output, /falling back to a filesystem walk/); + assert.equal( + status, + 1, + `the uncompiled file must still be found, got:\n${output}`, + ); + assert.match(output, /checked 1 eligible/); + assert.match(output, /src\/a\/uncompiled\.rs produced no coverage records/); +}); + +test("--all refuses to report success when it checked nothing", () => { + // Zero eligible files in whole-tree mode means the walk broke, not that there + // is nothing to verify. Exit 2 (environment error), not 0. + const cwd = fs.mkdtempSync( + path.join(os.tmpdir(), "openhuman-cov-presence-empty-"), + ); + fs.writeFileSync(path.join(cwd, "cov.info"), ""); + execFileSync("git", ["init", "-q", "."], { cwd }); + let status = 0; + let output = ""; + try { + execFileSync("bash", [script, path.join(cwd, "cov.info"), "--all"], { + cwd, + encoding: "utf8", + env: { ...process.env, ALLOWLIST: path.join(cwd, "absent.txt") }, + }); + } catch (err) { + status = err.status; + output = `${err.stdout ?? ""}${err.stderr ?? ""}`; + } + assert.equal(status, 2, `expected exit 2, got ${status}:\n${output}`); + assert.match(output, /checked 0 eligible files/); +}); + +test("--files may legitimately check nothing and still pass", () => { + // The counterpart: a PR touching only test sources has nothing to verify, and + // that is not an error. + const res = run( + { "src/a/thing_tests.rs": WITH_FN }, + [], + ["--files", "src/a/thing_tests.rs"], + ); + assert.equal(res.status, 0); + assert.match(res.output, /checked 0 eligible/); +}); + +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/__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 new file mode 100755 index 0000000000..1f73fdcd82 --- /dev/null +++ b/scripts/ci/assert-coverage-presence.sh @@ -0,0 +1,267 @@ +#!/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}" + +# 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 +} + +# 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$)' + +# Invocation help, printed to stdout for --help and to stderr on a usage error. +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}" + +# 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 \ + | 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 + # `git ls-files` is not reliably available here, and its failure mode is + # silent. In this repo's CI container it prints + # + # fatal: detected dubious ownership in repository at '/__w/openhuman/openhuman' + # + # because actions/checkout registers `safe.directory` under a temporarily + # overridden `HOME` that later steps do not run with. Read through a process + # substitution that produced an EMPTY candidate list, and this gate then + # reported "clean — every eligible changed source file produced coverage + # records" having checked ZERO files: the exact verified-nothing fail-open it + # exists to close, reproduced inside the fix for it. Observed on run + # 32367545922. + # + # So: take git's answer only if git actually succeeded AND returned something, + # otherwise walk the filesystem. The eligibility filter is identical either + # way; the only difference is that an untracked `src/**.rs` in a developer's + # working tree would also be checked, which is harmless (it is a real file + # that either compiled or did not). + listing="" + if listing="$(git ls-files 'src/*.rs' 'src/**/*.rs' 2>/dev/null)" && [ -n "${listing}" ]; then + log "enumerating tracked sources with git ls-files" + else + log "git ls-files unavailable or empty — falling back to a filesystem walk" + listing="$(find src -type f -name '*.rs' 2>/dev/null || true)" + fi + while IFS= read -r f; do + [ -n "${f}" ] && candidates+=("${f}") + done < <(printf '%s\n' "${listing}" | 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)" + +# A whole-tree run that checked nothing has not passed — it has failed to look. +# This repository always contains eligible Rust sources, so zero means the tree +# walk broke, and reporting success on it is the fail-open this gate exists to +# close. `--files` is exempt: a PR touching only tests or docs legitimately has +# nothing to check. +if [ "${MODE}" = all ] && [ "${checked}" -eq 0 ]; then + die "--all checked 0 eligible files. The tree walk found nothing, which for this repository means it failed rather than that there is nothing to verify. Refusing to report success." +fi + +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..c02c10cb0c --- /dev/null +++ b/scripts/ci/coverage-presence-allowlist.txt @@ -0,0 +1,19 @@ +# 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. +# +# It has already come off once. This file briefly carried +# `src/openhuman/hosting/{mod,tools}.rs`, because the gate was written before +# #5619 classified the `hosting` Cargo gate into `scripts/ci/product-features.txt`. +# #5619 merged first, the lane now compiles the family, and the two lines were +# deleted rather than left to rot into "excluded on purpose". +# +# (empty) diff --git a/scripts/ci/rust-coverage-changed.sh b/scripts/ci/rust-coverage-changed.sh index 8f855a9a83..803551baa7 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$##' | @@ -105,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 @@ -133,6 +174,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 +327,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