From 2e980b98e097443594be4347994371c1ab71d43b Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 21:31:08 +0200 Subject: [PATCH 1/2] ci: move seven job banners onto the jobs they describe, and gate it (fixes #621) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven of ci.yml's seventeen `# ── … ──` section banners sat above the *previous* job's trailing `sccache stats` / `Save sccache` steps, so each banner -- and for several jobs the multi-paragraph argument under it, which is the only written record of why that job exists -- described a job beginning 6-31 lines further down, behind another job's steps. `# ── Valgrind (memcheck) ──` was followed by three `linux-all-features` steps and only then by `valgrind:`. The move is comment-only. Verified against the base revision: banners old/new: 17 17 same multiset: True non-comment lines identical & in order: True 1133 1133 comment multiset identical: True 1173 1173 parsed YAML identical: True jobs: 18 so no job, step, key or expression changed -- only which lines the comments sit between. A displacement check on its own would have been vacuous: a file with every banner *deleted* scores a perfect zero displacements, which is this repository's named failure mode. scripts/check_workflow_job_banners.py therefore pairs both ways -- every banner must introduce a job, and every job must be introduced by a banner -- so deleting a banner fails rule B instead of satisfying rule A. Four jobs carry no banner today and are listed in UNBANNERED with a reason; the two hygiene rules from check_workflow_option_coverage.py's EXEMPT (an entry for a job that does not exist is an error, an entry for a job that now has a banner is an error) keep that list from rotting. Workflows with no banner at all are skipped, derived from the file rather than listed. Measured, not asserted: - Run against the pre-fix ci.yml the gate reports the seven displacements at 321, 711, 877, 967, 1363, 1641 and 1919 -- exactly the set #621 filed -- plus the seven now-bannerless jobs, and exits 1. - Run against this tree it exits 0 over all 24 banners in ci.yml and drift-guard.yml. - scripts/test_check_workflow_job_banners.sh drives twelve mutations: the founding displacement reproduced mechanically, a banner deleted rather than moved, a new job added without a banner, both UNBANNERED hygiene rules, the banner syntax reworded tree-wide (anti-vacuity), and four false-positive mirrors. All twelve pass. - python3 scripts/check_workflow_option_coverage.py . still exits 0 with the new job present. drift-guard.yml is the more natural home for the job and is where it belongs once PR #635 lands; it is held there now, so the job lives in ci.yml -- the same arrangement the option-coverage job is in. drift-guard.yml's own six banners were measured and are all correctly placed. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- .github/workflows/ci.yml | 158 ++++++++------ scripts/check_workflow_job_banners.py | 236 +++++++++++++++++++++ scripts/test_check_workflow_job_banners.sh | 202 ++++++++++++++++++ 3 files changed, 533 insertions(+), 63 deletions(-) create mode 100755 scripts/check_workflow_job_banners.py create mode 100755 scripts/test_check_workflow_job_banners.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2b17e7764..5e15f2c65 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -318,8 +318,6 @@ jobs: if: endsWith(matrix.preset, '-debug') run: ctest --preset ${{ matrix.preset }} - # ── Linux: sanitizers + coverage (all clang) ───────────────────────── - # Cumulative hit/miss for this leg. Without it the cache is # unfalsifiable: a thrashing cache and a working one look identical # from the outside, and only the build-step duration hints at which @@ -386,6 +384,7 @@ jobs: path: /home/runner/.cache/sccache key: sccache-${{ matrix.preset }}-${{ github.sha }} + # ── Linux: sanitizers + coverage (all clang) ───────────────────────── linux-sanitizers: name: Linux / ${{ matrix.preset }} needs: probe-self-hosted @@ -708,27 +707,6 @@ jobs: # forever, since the step now actually runs on every coverage build. retention-days: 14 - # ── Linux: kanban's concurrent-move stress test under ThreadSanitizer ── - # test_kanban_stress.cpp's [tsan]-tagged TEST_CASE drives BoardModel through - # a bare morph::bridge::Bridge/morph::backend::LocalBackend directly, on a - # real morph::exec::ThreadPoolExecutor{4}, with zero Qt frames anywhere in - # its call graph (see the test file's own header comment) -- so the "a GUI - # stack under TSan is mostly noise" rationale that keeps the ladder out of - # linux-sanitizers does not apply to this one test. This job builds only - # what that test needs -- MORPH_BUILD_LADDER=ON, MORPH_LADDER_RUNGS=kanban, - # no Qt GUI modules beyond the WebSockets backend the ladder testkit itself - # requires -- to keep it a minimal, fast, TSan-clean addition rather than - # pulling every rung's Qt Quick/QML code into the sanitizer matrix. - # - # (History: an earlier version of this test drove the same scenario through - # BackendRig{Mode::Local, ...}, whose Mode::Local unconditionally - # constructs a real morph::qt::QtExecutor for client-facing callback - # delivery -- morph#128 found 165 ThreadSanitizer warnings bottoming out in - # genuine Qt-internal frames reached through it, undetectable as real bugs - # or false positives from outside a TSan-instrumented Qt build. Rewriting - # the test to never construct a QtExecutor at all sidesteps the ambiguity - # entirely rather than resolving it.) - # Cumulative hit/miss for this leg. Without it the cache is # unfalsifiable: a thrashing cache and a working one look identical # from the outside, and only the build-step duration hints at which @@ -752,6 +730,26 @@ jobs: path: /home/runner/.cache/sccache key: sccache-clang-coverage-${{ github.sha }} + # ── Linux: kanban's concurrent-move stress test under ThreadSanitizer ── + # test_kanban_stress.cpp's [tsan]-tagged TEST_CASE drives BoardModel through + # a bare morph::bridge::Bridge/morph::backend::LocalBackend directly, on a + # real morph::exec::ThreadPoolExecutor{4}, with zero Qt frames anywhere in + # its call graph (see the test file's own header comment) -- so the "a GUI + # stack under TSan is mostly noise" rationale that keeps the ladder out of + # linux-sanitizers does not apply to this one test. This job builds only + # what that test needs -- MORPH_BUILD_LADDER=ON, MORPH_LADDER_RUNGS=kanban, + # no Qt GUI modules beyond the WebSockets backend the ladder testkit itself + # requires -- to keep it a minimal, fast, TSan-clean addition rather than + # pulling every rung's Qt Quick/QML code into the sanitizer matrix. + # + # (History: an earlier version of this test drove the same scenario through + # BackendRig{Mode::Local, ...}, whose Mode::Local unconditionally + # constructs a real morph::qt::QtExecutor for client-facing callback + # delivery -- morph#128 found 165 ThreadSanitizer warnings bottoming out in + # genuine Qt-internal frames reached through it, undetectable as real bugs + # or false positives from outside a TSan-instrumented Qt build. Rewriting + # the test to never construct a QtExecutor at all sidesteps the ambiguity + # entirely rather than resolving it.) kanban-tsan: name: Kanban / ThreadSanitizer runs-on: ubuntu-24.04 @@ -874,8 +872,6 @@ jobs: TSAN_OPTIONS: suppressions=${{ github.workspace }}/cmake/tsan.supp run: ctest --preset clang-tsan -L ladder-kanban -R ThreadSanitizer --output-on-failure - # ── Linux: Qt WebSocket backend build + tests ───────────────────────── - # Cumulative hit/miss for this leg. Without it the cache is # unfalsifiable: a thrashing cache and a working one look identical # from the outside, and only the build-step duration hints at which @@ -899,6 +895,7 @@ jobs: path: /home/runner/.cache/sccache key: sccache-clang-tsan-${{ github.sha }} + # ── Linux: Qt WebSocket backend build + tests ───────────────────────── linux-qt: name: Linux / Qt6 WebSockets runs-on: ubuntu-24.04 @@ -964,8 +961,6 @@ jobs: QT_QPA_PLATFORM: offscreen run: ctest --preset gcc-debug - # ── Linux: application ladder testkit (path-filtered) ───────────────── - # Cumulative hit/miss for this leg. Without it the cache is # unfalsifiable: a thrashing cache and a working one look identical # from the outside, and only the build-step duration hints at which @@ -989,6 +984,7 @@ jobs: path: /home/runner/.cache/sccache key: sccache-gcc-debug-${{ github.sha }} + # ── Linux: application ladder testkit (path-filtered) ───────────────── ladder-tests: name: Application ladder needs: probe-self-hosted @@ -1360,30 +1356,6 @@ jobs: python3 scripts/scenario/run_scenarios.py \ --build-dir build/gcc-debug "${rungs[@]}" - # ── Linux: every rung's tests under AddressSanitizer + UBSan ────────── - # Before this job, no rung test ran under any sanitizer: ladder-tests above - # builds plain gcc-debug, and the linux-sanitizers matrix deliberately skips - # the ladder (only its clang-coverage leg sets MORPH_BUILD_LADDER). So every - # rung's models, presenters and QML adapters — the code the ladder exists to - # exercise — were compiled and run with no memory or UB instrumentation - # anywhere in CI. - # - # ASan and UBSan, not TSan. `apply_sanitizers( asan)` compiles with - # -fsanitize=address,undefined (cmake/compiler_options.cmake), so this one - # preset delivers both and a separate ubsan leg for the ladder would re-run - # a strict subset. TSan is deliberately absent: a rung's tests drive Qt on - # every path, and against an uninstrumented system Qt that produces warnings - # bottoming out in Qt-internal frames which cannot be classified as real - # races or false positives from outside a TSan-instrumented Qt build — - # morph#128 hit exactly that, 165 warnings deep. The resolution there was to - # rewrite the one test that mattered to construct no QtExecutor at all and - # run only it under TSan, which is what kanban-tsan above does. Thread- - # sanitising a rung means following that pattern per test, not adding a - # blanket -DAF_SANITIZER=tsan leg here. - # - # -LE stress mirrors ladder-tests: the stress cases are long-running by - # design and ASan's shadow-memory overhead compounds that. - # Cumulative hit/miss for this leg. Without it the cache is # unfalsifiable: a thrashing cache and a working one look identical # from the outside, and only the build-step duration hints at which @@ -1448,6 +1420,29 @@ jobs: path: /home/runner/.cache/sccache key: sccache-gcc-debug-${{ github.sha }} + # ── Linux: every rung's tests under AddressSanitizer + UBSan ────────── + # Before this job, no rung test ran under any sanitizer: ladder-tests above + # builds plain gcc-debug, and the linux-sanitizers matrix deliberately skips + # the ladder (only its clang-coverage leg sets MORPH_BUILD_LADDER). So every + # rung's models, presenters and QML adapters — the code the ladder exists to + # exercise — were compiled and run with no memory or UB instrumentation + # anywhere in CI. + # + # ASan and UBSan, not TSan. `apply_sanitizers( asan)` compiles with + # -fsanitize=address,undefined (cmake/compiler_options.cmake), so this one + # preset delivers both and a separate ubsan leg for the ladder would re-run + # a strict subset. TSan is deliberately absent: a rung's tests drive Qt on + # every path, and against an uninstrumented system Qt that produces warnings + # bottoming out in Qt-internal frames which cannot be classified as real + # races or false positives from outside a TSan-instrumented Qt build — + # morph#128 hit exactly that, 165 warnings deep. The resolution there was to + # rewrite the one test that mattered to construct no QtExecutor at all and + # run only it under TSan, which is what kanban-tsan above does. Thread- + # sanitising a rung means following that pattern per test, not adding a + # blanket -DAF_SANITIZER=tsan leg here. + # + # -LE stress mirrors ladder-tests: the stress cases are long-running by + # design and ASan's shadow-memory overhead compounds that. ladder-sanitizers: name: Application ladder / ASan+UBSan needs: probe-self-hosted @@ -1638,16 +1633,6 @@ jobs: ctest --preset clang-asan --no-tests=error --output-on-failure \ -R 'morph::qt::|qt_tls_example_runs' - # ── Linux: every optional feature enabled at once ───────────────────── - # Every MORPH_BUILD_* option below is off by default, and until this job - # existed no CI configuration turned any of them on — so several thousand - # lines (morph::net, the Qt/QML forms renderer, the SQLite queue, the fuzz - # harnesses, the vetted-HMAC adapters, the soak/bench targets) were never - # compiled here, let alone tested. That is how a build that cannot configure - # (MORPH_REQUIRE_VETTED_HMAC) and a replay test matching the wrong file - # extension both reached master green. Enabling them together also proves - # they compose, which building each alone would not. - # Cumulative hit/miss for this leg. Without it the cache is # unfalsifiable: a thrashing cache and a working one look identical # from the outside, and only the build-step duration hints at which @@ -1712,6 +1697,15 @@ jobs: path: /home/runner/.cache/sccache key: sccache-clang-asan-${{ github.sha }} + # ── Linux: every optional feature enabled at once ───────────────────── + # Every MORPH_BUILD_* option below is off by default, and until this job + # existed no CI configuration turned any of them on — so several thousand + # lines (morph::net, the Qt/QML forms renderer, the SQLite queue, the fuzz + # harnesses, the vetted-HMAC adapters, the soak/bench targets) were never + # compiled here, let alone tested. That is how a build that cannot configure + # (MORPH_REQUIRE_VETTED_HMAC) and a replay test matching the wrong file + # extension both reached master green. Enabling them together also proves + # they compose, which building each alone would not. linux-all-features: name: Linux / all optional features (${{ matrix.compiler }}) needs: probe-self-hosted @@ -1916,8 +1910,6 @@ jobs: done exit "$status" - # ── Valgrind (memcheck) ─────────────────────────────────────────────── - # Cumulative hit/miss for this leg. Without it the cache is # unfalsifiable: a thrashing cache and a working one look identical # from the outside, and only the build-step duration hints at which @@ -1982,6 +1974,7 @@ jobs: path: /home/runner/.cache/sccache key: sccache-${{ matrix.preset }}-${{ github.sha }} + # ── Valgrind (memcheck) ─────────────────────────────────────────────── valgrind: name: Valgrind memcheck runs-on: ubuntu-24.04 @@ -2446,6 +2439,45 @@ jobs: - name: Check every declared MORPH_BUILD_* option is enabled by some job run: python3 scripts/check_workflow_option_coverage.py . + # ── Every job's section banner introduces that job ──────────────────── + # + # Its own job, and deliberately a cheap one: it reads workflow files and + # builds nothing, so it reports in seconds rather than riding on a leg that + # takes 26-74 minutes to say the same thing. + # + # (drift-guard.yml is the more natural home and is where this belongs once + # PR #635 lands; it is held there now, so the job lives here -- the same + # arrangement the option-coverage job above is in.) + # + # Catches morph#621's bug class: seven of the seventeen banners in this file + # sat above the *previous* job's trailing `sccache stats` / `Save sccache` + # steps, so each banner's multi-paragraph rationale -- for several jobs the + # only written record of why the job exists -- described a job that began + # 6-31 lines further down, behind another job's steps. YAML comments are + # inert, so nothing else in this repository can notice, and the displacement + # recurs every time a step is appended to the end of a job. + # + # Checking "no banner is displaced" alone would be vacuous: a file with every + # banner deleted scores zero displacements. The gate pairs both ways -- every + # banner introduces a job, and every job is introduced by a banner -- so a + # deletion fails it instead of satisfying it. + banner-lint: + name: Every job's section banner introduces that job + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v4 + + # See scripts/test_check_workflow_job_banners.sh's own comment: a lint + # gate nobody tests reports green whether or not it still detects + # anything, and this one repairs the defect it guards in the same commit + # that adds it, so it passes on day one whether it parses anything at + # all. + - name: Self-test the job-banner checker + run: bash scripts/test_check_workflow_job_banners.sh + + - name: Check every workflow section banner introduces its job + run: python3 scripts/check_workflow_job_banners.py . + # ── Install / export: find_package(morph CONFIG) must work ───────────── # # Its own job rather than a step on an existing leg. It configures, installs diff --git a/scripts/check_workflow_job_banners.py b/scripts/check_workflow_job_banners.py new file mode 100755 index 000000000..83d1c322c --- /dev/null +++ b/scripts/check_workflow_job_banners.py @@ -0,0 +1,236 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +"""Usage: python3 scripts/check_workflow_job_banners.py [REPO_ROOT] + +Fails if a ` # ── … ──` section banner in a workflow does not introduce the +job it describes, or if a job that should carry one has lost it. + +Why this gate exists (morph#621): seven of ci.yml's seventeen banners sat above +the *previous* job's trailing `sccache stats` / `Save sccache` steps, so the +banner and its multi-paragraph rationale described a job that began 6-31 lines +further down, behind another job's steps. `# ── Valgrind (memcheck) ──` was +followed by three steps belonging to `linux-all-features` and only then by +`valgrind:`. These banners are not decoration: for several jobs the paragraphs +under the banner are the only written record of why the job exists at all +(`# ── Linux: every rung's tests under AddressSanitizer + UBSan ──` carries ~30 +lines on why ASan and UBSan but not TSan, citing morph#128), and a reader who +scrolls into that argument attributes it to whichever job's steps surround it. + +YAML comments are inert, so nothing else in this repository can notice. The +displacement accumulated because cache steps were appended to each job after +the next job's banner already existed, and were inserted above it rather than +below -- which will happen again the next time a step is appended to the end of +a job. + +## The check this deliberately is *not* + +Checking only "no banner is displaced" would be vacuous: a tree with every +banner **deleted** scores a perfect zero displacements. That is this +repository's named failure mode -- a control that reports success while +measuring nothing -- so the check is a two-way pairing rather than a one-way +scan: + + A. Every banner must introduce a job. Skipping its own continuation comment + lines and blank lines, the next line after a banner must be a top-level + `:` key. This is the displacement catcher. + + B. Every job must be introduced by a banner. A job with no banner is an + error unless it is in UNBANNERED below with a written reason. This is what + makes rule A non-vacuous: delete a banner to satisfy A and B fails. + +Two rules keep UNBANNERED from becoming a stale list, copied from +scripts/check_workflow_option_coverage.py's EXEMPT: an entry for a job that +does not exist is an error, and an entry for a job that *does* have a banner is +an error. An exemption has to be necessary to be allowed to stay. + +## Scope + +Workflows that contain no banner at all are skipped, and that is derived from +the file rather than listed here: docs.yml, mutation.yml, spec-sync.yml, +suppression-guard.yml and the two wasm workflows are single-job files that have +never used the banner style, and a gate that demanded they adopt it would be +inventing a convention rather than enforcing one. A file that adopts its first +banner opts into both rules for all of its jobs. + +Finding zero banners across the whole tree is a failure, not a pass -- a gate +with nothing left to check reports green exactly as loudly as one that checked +everything. + +Reads text only; compiles and runs nothing. +""" + +from __future__ import annotations + +import re +import sys +from pathlib import Path + +# Jobs allowed to carry no section banner, and why. The reason is the payload. +# +# Keyed by workflow file name, then job id. Listing the jobs *positively* -- +# rather than deriving "everything before the file's first banner is a preamble" +# -- is deliberate: under that derivation, deleting a file's *first* banner +# would silently reclassify its job as preamble and the deletion would go +# unreported, which is exactly the hole rule B exists to close. +UNBANNERED: dict[str, dict[str, str]] = { + "ci.yml": { + "probe-self-hosted": "not a section of the build matrix but the runner " + "selection that every Linux leg below consumes through `needs:`; its " + "rationale is written as a plain comment block because it introduces no " + "section", + }, + "drift-guard.yml": { + "sanitizer-can-fail": "part of the file's un-sectioned preamble: the " + "first three jobs predate the `# ──` sectioning that starts at the " + "ladder rung-list gate", + "dep-cache-selftest": "part of the file's un-sectioned preamble (see " + "sanitizer-can-fail)", + "prose-lint": "part of the file's un-sectioned preamble (see " + "sanitizer-can-fail)", + }, +} + +# A banner: a top-level (two-space) comment opening with the box-drawing rule. +BANNER_RE = re.compile(r"^ # ── ") +# A top-level comment line, banner or continuation. Six-space comments belong +# to steps, not to sections, and must not be skipped over. +TOP_COMMENT_RE = re.compile(r"^ #") +# A top-level job key. `jobs:` itself is at column 0, so two spaces is a job. +JOB_KEY_RE = re.compile(r"^ ([A-Za-z0-9_-]+):\s*(#.*)?$") + + +def job_keys(lines: list[str]) -> list[tuple[int, str]]: + """(index, job-id) for every top-level key under `jobs:`.""" + out: list[tuple[int, str]] = [] + in_jobs = False + for i, line in enumerate(lines): + if re.match(r"^jobs:\s*(#.*)?$", line): + in_jobs = True + continue + if in_jobs and line and not line[0].isspace(): + in_jobs = False + if not in_jobs: + continue + m = JOB_KEY_RE.match(line) + if m: + out.append((i, m.group(1))) + return out + + +def introduced_job(lines: list[str], banner: int) -> tuple[int, str] | None: + """The job a banner introduces, or None when the banner is displaced. + + Walks forward past the banner's own continuation comments and blank lines. + """ + j = banner + 1 + while j < len(lines) and (not lines[j].strip() or TOP_COMMENT_RE.match(lines[j])): + j += 1 + if j >= len(lines): + return None + m = JOB_KEY_RE.match(lines[j]) + return (j, m.group(1)) if m else None + + +def main(argv: list[str]) -> int: + root = Path(argv[1] if len(argv) > 1 else Path(__file__).resolve().parent.parent) + workflows = sorted((root / ".github" / "workflows").glob("*.yml")) + if not workflows: + print(f"error: no workflows found under {root}/.github/workflows", file=sys.stderr) + return 1 + + failures = 0 + banners_seen = 0 + + def fail(msg: str) -> None: + nonlocal failures + print(f"error: {msg}", file=sys.stderr) + failures += 1 + + for path in workflows: + rel = path.relative_to(root) + lines = path.read_text(encoding="utf-8").split("\n") + banners = [i for i, line in enumerate(lines) if BANNER_RE.match(line)] + if not banners: + print(f"ok: {rel}: no section banners, not in the banner style") + continue + + banners_seen += len(banners) + jobs = job_keys(lines) + if not jobs: + fail(f"{rel}: {len(banners)} section banner(s) but no top-level job keys") + continue + + exempt = UNBANNERED.get(path.name, {}) + bannered: dict[str, int] = {} + + # -- Rule A: every banner introduces a job --------------------------- + for b in banners: + target = introduced_job(lines, b) + if target is None: + landed = next( + ( + f"{k + 1}: {lines[k].strip()}" + for k in range(b + 1, len(lines)) + if lines[k].strip() and not TOP_COMMENT_RE.match(lines[k]) + ), + "end of file", + ) + fail( + f"{rel}:{b + 1}: this section banner does not introduce a job --\n" + f" {lines[b].strip()}\n" + f" the next non-comment line is {landed}\n" + f" Move the banner (and the paragraphs under it) down to sit\n" + f" directly above the `:` key it describes." + ) + continue + _, job = target + bannered[job] = b + print(f"ok: {rel}:{b + 1}: introduces `{job}`") + + # -- Rule B: every job is introduced by a banner --------------------- + job_ids = {name for _, name in jobs} + for idx, name in jobs: + if name in bannered: + continue + if name in exempt: + print(f"ok: {rel}:{idx + 1}: `{name}` bannerless by exemption") + continue + fail( + f"{rel}:{idx + 1}: job `{name}` is introduced by no section banner.\n" + f" Every job in a workflow that uses ` # ── ` banners needs one:\n" + f" without this rule, deleting a banner outright would satisfy the\n" + f" displacement check instead of failing it. Add the banner, or add\n" + f" `{name}` to UNBANNERED in {Path(__file__).name} with a reason." + ) + + # -- UNBANNERED hygiene ---------------------------------------------- + for name in exempt: + if name not in job_ids: + fail( + f"{rel}: UNBANNERED names `{name}`, which is not a job in this " + f"workflow. Remove the stale entry." + ) + elif name in bannered: + fail( + f"{rel}: UNBANNERED names `{name}`, but it now has a banner at " + f"line {bannered[name] + 1}. Remove the unnecessary entry." + ) + + # -- Anti-vacuity -------------------------------------------------------- + if banners_seen == 0: + fail( + "no ` ── ` section banner found in any workflow. Either the " + "convention was abandoned or it was reworded out of the shape this gate " + "reads -- both leave the gate checking nothing while still exiting 0." + ) + + if failures: + print(f"\n{failures} workflow job-banner check(s) failed", file=sys.stderr) + return 1 + + print(f"\nok: all {banners_seen} section banner(s) introduce the job they describe") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/scripts/test_check_workflow_job_banners.sh b/scripts/test_check_workflow_job_banners.sh new file mode 100755 index 000000000..722bf1be8 --- /dev/null +++ b/scripts/test_check_workflow_job_banners.sh @@ -0,0 +1,202 @@ +#!/usr/bin/env bash +# Usage: bash scripts/test_check_workflow_job_banners.sh +# +# Self-test for scripts/check_workflow_job_banners.py, the gate that keeps every +# ` # ── … ──` section banner sitting above the job it describes (morph#621). +# +# A lint gate nobody tests reports green whether or not it still detects +# anything, and this one is maximally exposed to that: the seven displacements +# morph#621 reported are repaired in the same commit that adds the gate, so the +# gate passes on day one whether it parses anything at all. +# +# It is also the gate whose *vacuous* form is easiest to write by accident. A +# checker that only counted displacements would score a perfect zero on a tree +# with every banner deleted. So the deletion case below is not an extra: it is +# the case that decides whether this gate measures anything. +# +# Every drift the gate claims to catch is reintroduced into a scratch copy of +# the tree, one at a time, and must be caught for the stated reason. One +# mutation at a time matters: applied together, a single detection would mask +# every other. +set -euo pipefail + +readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +readonly checker="scripts/check_workflow_job_banners.py" + +failures=0 + +note() { printf 'ok: %s\n' "$*"; } +fail() { printf 'error: %s\n' "$*" >&2; failures=$((failures + 1)); } + +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +# The checker reads .github/workflows/*.yml and its own UNBANNERED table; a +# copy of those two paths is the whole tree it needs. +readonly pristine="${scratch}/pristine" +mkdir -p "${pristine}/.github/workflows" "${pristine}/scripts" +cp "${repo_root}"/.github/workflows/*.yml "${pristine}/.github/workflows/" +cp "${repo_root}/${checker}" "${pristine}/scripts/" + +make_tree() { + local dest="$1" + rm -rf "$dest" + mkdir -p "$dest" + cp -R "${pristine}/." "$dest" +} + +# `sed -i` is not portable between GNU and BSD sed; edit through a temp file. +edit() { + local file="$1"; shift + sed "$@" "$file" > "${file}.new" + mv "${file}.new" "$file" +} + +# Reproduce morph#621's defect mechanically: lift the banner line matching +# $2 out of $1 and re-insert it $3 lines earlier, i.e. back into the previous +# job's trailing steps. Done in Python because it is a line move, which sed +# cannot express without holding the whole file in the hold space. +displace_banner() { + python3 - "$1" "$2" "$3" <<'PY' +import sys +path, needle, up = sys.argv[1], sys.argv[2], int(sys.argv[3]) +lines = open(path, encoding="utf-8").read().split("\n") +hits = [i for i, l in enumerate(lines) if l.startswith(" # ── ") and needle in l] +assert len(hits) == 1, f"{needle!r} matched {len(hits)} banners" +i = hits[0] +banner = lines.pop(i) +lines.insert(i - up, banner) +open(path, "w", encoding="utf-8").write("\n".join(lines)) +PY +} + +# Each mutation must be caught, and caught *for the stated reason*. `$expected` +# is a substring the diagnostic must contain; without it a mutation that broke +# the tree some unrelated way -- a mangled sed, a file the mutator emptied -- +# would count as a detection, and this self-test would report a gate that no +# longer detects anything as fully working. +expect_caught() { + local description="$1" mutator="$2" expected="$3" + local tree="${scratch}/case" output + make_tree "$tree" + if ! ( cd "$tree" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "$tree" && python3 "$checker" . 2>&1 )"; then + fail "NOT caught: ${description} -- the gate passed a tree it should reject" + printf '%s\n' "$output" >&2 + return + fi + if printf '%s' "$output" | grep -qF "$expected"; then + note "caught: ${description}" + else + fail "caught for the WRONG reason: ${description} -- no diagnostic containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +# The mirror, for false positives. A gate that rejected every tree would +# "catch" every case below while being worthless. +expect_accepted() { + local description="$1" mutator="$2" + local tree="${scratch}/case" output + make_tree "$tree" + if ! ( cd "$tree" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "$tree" && python3 "$checker" . 2>&1 )"; then + note "accepted: ${description}" + else + fail "FALSE POSITIVE: ${description} -- the gate rejected a tree it should accept:" + printf '%s\n' "$output" >&2 + fi +} + +# -- The unmodified tree must pass ------------------------------------------- +make_tree "${scratch}/clean" +if output="$( cd "${scratch}/clean" && python3 "$checker" . 2>&1 )"; then + note "the unmodified tree passes" +else + fail "the unmodified tree was rejected by the gate:" + printf '%s\n' "$output" >&2 +fi + +# -- The defect this gate exists for ----------------------------------------- +# morph#621 exactly: the banner pushed back above the previous job's trailing +# `sccache stats` / `Save sccache` steps. Eight lines is enough to clear the +# `valgrind:` key and land inside `linux-all-features`'s cache steps. +expect_caught "a banner displaced into the previous job's trailing steps" \ + "displace_banner .github/workflows/ci.yml 'Valgrind (memcheck)' 8" \ + "this section banner does not introduce a job" + +# The same for one with a multi-paragraph rationale under it: only the banner +# line moves, so the paragraphs stay behind and the gate must still object. +expect_caught "a banner with a rationale block displaced upward" \ + "displace_banner .github/workflows/ci.yml 'every optional feature enabled at once' 12" \ + "this section banner does not introduce a job" + +# -- The vacuity case -------------------------------------------------------- +# This is the case that decides whether the gate measures anything. Deleting a +# banner outright removes a displacement, so a checker that only counted +# displacements reports this tree as *more* correct than the real one. +expect_caught "a banner deleted rather than moved" \ + "edit .github/workflows/ci.yml -e '/# ── Valgrind (memcheck)/d'" \ + "job \`valgrind\` is introduced by no section banner" + +# The same defect one step later: a job added to a bannered workflow without a +# banner. This is the day-that-has-not-happened-yet case -- the gate's value is +# what it does then, not on the tree it shipped with. +expect_caught "a new job added to a bannered workflow with no banner" \ + "printf '%s\n' ' brand-new-lint:' ' runs-on: ubuntu-24.04' ' steps:' ' - uses: actions/checkout@v4' >> .github/workflows/ci.yml" \ + "job \`brand-new-lint\` is introduced by no section banner" + +# -- The exemption set must stay necessary ----------------------------------- +# An exemption for a job that does have a banner: the hand-written record that +# is no longer true and that nothing reads back against reality. +expect_caught "an exemption for a job that now has a banner" \ + "edit ${checker} -e 's|^ \"probe-self-hosted\": | \"valgrind\": \"stale\",\n \"probe-self-hosted\": |'" \ + "but it now has a banner at line" + +# An exemption for a job that no longer exists. +expect_caught "an exemption naming a job the workflow does not declare" \ + "edit ${checker} -e 's|^ \"probe-self-hosted\": | \"long-gone\": \"stale\",\n \"probe-self-hosted\": |'" \ + "which is not a job in this workflow" + +# -- The gate must not go blind ---------------------------------------------- +# If the banner syntax is reworded out from under the parser, every workflow +# reads as "not in the banner style" and both rules stop applying. The honest +# answer is failure, not the silent green a per-file skip would produce. +expect_caught "the banner syntax is reworded tree-wide" \ + "edit .github/workflows/ci.yml -e 's|^ # ── | # == |' \ + && edit .github/workflows/drift-guard.yml -e 's|^ # ── | # == |'" \ + "section banner found in any workflow" + +# -- False positives --------------------------------------------------------- +# The gate is about placement, not wording. Rewriting what a banner says must +# not be drift, or every rename would have to touch this script. +expect_accepted "a banner's text rewritten in place" \ + "edit .github/workflows/ci.yml -e 's|^ # ── Valgrind (memcheck) .*| # ── Valgrind, under memcheck ──|'" + +# A banner separated from its job by blank lines and by its own continuation +# paragraphs. Both shapes are already in ci.yml; the gate must accept both, or +# it would be pinning a whitespace convention the tree does not follow. +expect_accepted "blank lines and rationale paragraphs between banner and job" \ + "edit .github/workflows/ci.yml -e 's|^ valgrind:| #\n # An added paragraph.\n\n valgrind:|'" + +# A whole new job introduced by its own banner: the ordinary way the workflow +# grows. If this failed, the gate would be a tax on adding jobs. +expect_accepted "a new job added together with its banner" \ + "printf '%s\n' '' ' # ── A brand new lint ──' ' brand-new-lint:' ' runs-on: ubuntu-24.04' ' steps:' ' - uses: actions/checkout@v4' >> .github/workflows/ci.yml" + +# A workflow that has never used the banner style must not be dragged into it. +expect_accepted "a job added to a workflow with no banners at all" \ + "printf '%s\n' '' ' another-docs-job:' ' runs-on: ubuntu-24.04' ' steps:' ' - uses: actions/checkout@v4' >> .github/workflows/docs.yml" + +if [ "$failures" -ne 0 ]; then + printf '\n%d case(s) failed.\n' "$failures" >&2 + exit 1 +fi + +printf '\nall cases passed.\n' From 490e1140766129574bb371c9db01c6add0fd992f Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 22:12:32 +0200 Subject: [PATCH 2/2] lint: record and gate the reach tests/.clang-tidy actually has (fixes #632) clang-tidy resolves its configuration from the path of the translation unit it analyses, not from the path of the file a diagnostic lands in. tests/.clang-tidy therefore scopes which TUs it governs, not which files their diagnostics may name: its thirteen suppressions -- each argued as Catch2 or raw-syscall idiom, correctly, about test code -- are also off for every include/morph/** header those TUs reach. A header gate driven by test TUs is green on findings it was built to catch. #632 confirmed the mechanism on one case and did not size it. Measured here, on a8511aa6 with clang-tidy 22.1.8, over the 134 tests/ TUs in the compile database the clang-tidy-diff job configures for itself (every optional feature ON; 703 entries, 134 distinct tests/ TUs, all 134 analysed, zero clang-diagnostic-error in either run): distinct findings inside include/morph/**, from those 134 TUs with tests/.clang-tidy in place .... 262 with it removed .................... 595 hidden by it ....................... 333, across 25 headers 247 cppcoreguidelines-pro-bounds-avoid-unchecked-container-access 57 readability-identifier-length 6 cppcoreguidelines-pro-bounds-array-to-pointer-decay 5 modernize-avoid-c-arrays 5 cppcoreguidelines-avoid-c-arrays 5 bugprone-unchecked-optional-access 3 cppcoreguidelines-pro-type-vararg 3 readability-convert-member-functions-to-static 2 readability-function-cognitive-complexity The set difference is one-directional: 0 findings appear with the file present that do not appear with it removed, which is what the mechanism predicts. The reach cannot be narrowed -- clang-tidy offers no way to scope a suppression to the file a diagnostic lands in -- so this takes #632's third close: write it down, and gate the record so it cannot go stale. - tests/.clang-tidy gains a section stating the reach, the measurement above, and a machine-readable `header-reach:` list naming all thirteen checks a test-TU-driven gate cannot report. - scripts/check_tidy_suppression_scope.sh checks both halves. Textually, that `header-reach:` names exactly what `Checks:` subtracts. Behaviourally, that the reach still exists: a probe header under include/morph/ is analysed from a TU under tests/ and from one that is not, using this repository's real .clang-tidy files, and the finding must be absent from the first and present from the second. The second probe is the anti-vacuity control -- the first assertion is a negative, which a probe that produces no findings at all satisfies by accident. Behavioural rather than a grep so that the note goes red rather than quietly stale if clang-tidy ever resolves configuration per diagnostic file. - Both run in the existing clang-tidy job, the only job that already has a pinned clang-tidy, before the Configure step: neither reads a compile database. Verified: - scripts/test_check_tidy_suppression_scope.sh drives nine cases -- a suppression added without a record entry, a record entry with no suppression, a suppression removed but recorded, the Checks: block reworded out from under the parser, the probed check ceasing to be suppressed, the probe rendered unable to fire from anywhere, and two false-positive mirrors. All nine pass. - The probe reproduces #632's mechanism independently of the sweep above: the same two checks are reported against include/morph/probe_scope.hpp from src/probe_scope_tu.cpp and not from tests/probe_scope_tu.cpp. On the nine examples/*/tests/ copies: #632 states they "carry the same entries". They do not -- each subtracts exactly one check, bugprone-chained-comparison. Their reach is the same but its effect is nil: that check fires only on Catch2's REQUIRE expansion and is attributed to the test source line. Measured over the 154 example test TUs in the same database, findings inside include/morph/** were identical with and without those nine files, 312 either way -- but that run cannot separate "no header reach" from "the check never fired", because Catch2 3.16.0 locally carries the NOLINT and bugprone-chained-comparison produced zero findings in either run. The structural argument carries the conclusion; the measurement only fails to contradict it. Recorded as such in tests/.clang-tidy, and noted on #632. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- .github/workflows/ci.yml | 25 +++ scripts/check_tidy_suppression_scope.sh | 197 +++++++++++++++++++ scripts/test_check_tidy_suppression_scope.sh | 167 ++++++++++++++++ tests/.clang-tidy | 90 ++++++++- 4 files changed, 474 insertions(+), 5 deletions(-) create mode 100755 scripts/check_tidy_suppression_scope.sh create mode 100755 scripts/test_check_tidy_suppression_scope.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5e15f2c65..7fdcbc709 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2179,6 +2179,31 @@ jobs: wget -qO- https://apt.llvm.org/llvm.sh | sudo bash -s -- ${{ env.CLANG_VERSION }} sudo apt-get install -y clang-tidy-${{ env.CLANG_VERSION }} + # Here rather than in a lint job of its own because this is the only job + # that already has a pinned clang-tidy on it, and both steps below need + # one. Neither reads the compile database, so they run before the + # Configure step and fail in seconds rather than after it. + # + # See scripts/test_check_tidy_suppression_scope.sh's own comment: a lint + # gate nobody tests reports green whether or not it still detects + # anything, and this one's behavioural half asserts a *negative* -- that + # a check does not fire in a header -- which a broken probe satisfies by + # accident. + - name: Self-test the clang-tidy suppression-scope checker + run: bash scripts/test_check_tidy_suppression_scope.sh clang-tidy-${{ env.CLANG_VERSION }} + + # Catches morph#632's bug class: tests/.clang-tidy's thirteen + # suppressions are argued as Catch2 and raw-syscall idiom, which is true + # of test sources and says nothing about include/morph/** -- yet + # clang-tidy resolves configuration from the translation unit's path, so + # they are off for every header a tests/ TU reaches as well. Measured on + # a8511aa6: 333 findings inside include/morph/**, across 25 headers, + # reported from those 134 TUs only once that file is removed. The reach + # cannot be narrowed, so it is written down in tests/.clang-tidy -- and + # this step is what keeps the record from going stale. + - name: Check tests/.clang-tidy records the reach it actually has + run: bash scripts/check_tidy_suppression_scope.sh clang-tidy-${{ env.CLANG_VERSION }} . + # See the linux-all-features job: the QML renderer needs Qt 6.5+. - name: Install Qt ${{ env.QT_VERSION }} uses: jurplel/install-qt-action@v4 diff --git a/scripts/check_tidy_suppression_scope.sh b/scripts/check_tidy_suppression_scope.sh new file mode 100755 index 000000000..f1029d019 --- /dev/null +++ b/scripts/check_tidy_suppression_scope.sh @@ -0,0 +1,197 @@ +#!/usr/bin/env bash +# Usage: bash scripts/check_tidy_suppression_scope.sh [CLANG_TIDY_BINARY] [REPO_ROOT] +# +# Keeps tests/.clang-tidy's record of its own reach true. +# +# Why this gate exists (morph#632): clang-tidy resolves its configuration from +# the path of the translation unit it is analysing, not from the path of the +# file a diagnostic lands in. tests/.clang-tidy's thirteen suppressions are +# each argued as *test idiom* -- Catch2's REQUIRE expansion, a raw-syscall +# harness -- and that argument is correct for test sources and says nothing +# about include/morph/**. But the suppressions apply there too, whenever the +# header is reached from a TU under tests/. A finding's visibility therefore +# depends on which TU happened to reach it, and a header gate driven by test +# TUs is green on findings it was built to catch. +# +# The reach was measured rather than assumed: over the 134 tests/ TUs in the +# clang-tidy job's own compile database, distinct findings inside +# include/morph/** went 262 with tests/.clang-tidy present to 595 with it +# removed -- 333 hidden across 25 headers. The numbers and the per-check split +# are recorded in tests/.clang-tidy's own header. +# +# A record nothing reads back is the thing this repository distrusts, so this +# gate checks both halves of that record: +# +# A. Behaviourally, that the reach still exists. A probe header under +# include/morph/ is compiled from a TU under tests/ and from a TU that is +# not, using this repository's *real* .clang-tidy files, and the finding +# must be absent from the first and present from the second. Behavioural +# rather than a grep for the mechanism, for the reason morph#298 +# established -- and this direction matters twice over: if clang-tidy ever +# resolves configuration per diagnostic file, the note goes red rather +# than quietly stale. +# +# The second probe is the anti-vacuity control. Without it, assertion A +# would also pass against a broken probe that produces no findings at all, +# which is exactly the failure this whole issue is about. +# +# B. Textually, that the `header-reach:` list in tests/.clang-tidy's header +# still names exactly the checks its `Checks:` key disables. Adding a +# fourteenth suppression without extending the list is the way the record +# goes stale, and it is the only way a reader finds out which checks a +# test-TU-driven gate cannot report. +# +# Finding zero suppressions in tests/.clang-tidy is a failure, not a pass: a +# gate with nothing left to check reports green exactly as loudly as one that +# checked everything. +# +# Needs a clang-tidy binary; compiles nothing and reads no compile database. +set -euo pipefail + +readonly tidy="${1:-clang-tidy}" +readonly repo_root="${2:-$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)}" + +readonly tests_config="${repo_root}/tests/.clang-tidy" +readonly root_config="${repo_root}/.clang-tidy" + +# The two checks the probe drives. Both are in tests/.clang-tidy's list, both +# are cheap to trigger from a header, and together they are 304 of the 333 +# findings measured as hidden. +readonly probe_checks=( + "cppcoreguidelines-pro-bounds-avoid-unchecked-container-access" + "readability-identifier-length" +) + +failures=0 +note() { printf 'ok: %s\n' "$*"; } +fail() { printf 'error: %s\n' "$*" >&2; failures=$((failures + 1)); } + +for f in "$tests_config" "$root_config"; do + if [ ! -f "$f" ]; then + printf 'error: %s not found\n' "$f" >&2 + exit 1 + fi +done + +if ! command -v "$tidy" > /dev/null 2>&1; then + printf 'error: clang-tidy binary %s not found on PATH\n' "$tidy" >&2 + exit 1 +fi + +# -- The two lists ----------------------------------------------------------- +# Everything `Checks:` subtracts, one per line, sorted. The folded block ends +# at the next unindented key. +disabled="$(awk ' + /^Checks:/ { in_block = 1; next } + in_block && /^[^[:space:]]/ { in_block = 0 } + in_block { print } +' "$tests_config" \ + | tr -d ' ' | tr ',' '\n' \ + | sed -n 's/^-\([A-Za-z0-9.-]\+\)$/\1/p' | sort -u)" + +# Everything the header comment records as unreportable in a header. +recorded="$(sed -n 's/^#[[:space:]]*header-reach:[[:space:]]*\([A-Za-z0-9.-]\+\)[[:space:]]*$/\1/p' \ + "$tests_config" | sort -u)" + +disabled_n="$(printf '%s' "$disabled" | grep -c . || true)" +recorded_n="$(printf '%s' "$recorded" | grep -c . || true)" + +if [ "$disabled_n" -eq 0 ]; then + fail "tests/.clang-tidy subtracts no checks at all -- either the file stopped + suppressing anything (in which case delete it and this gate), or its + \`Checks:\` block was reworded out of the shape this gate reads. Both leave + the gate checking nothing while still exiting 0." +else + note "tests/.clang-tidy subtracts ${disabled_n} check(s)" +fi + +if [ "$disabled" != "$recorded" ]; then + fail "tests/.clang-tidy's \`header-reach:\` list does not match what its + \`Checks:\` key subtracts. Every suppression there is also off for every + include/morph/** header reached from a TU under tests/ (morph#632), and the + list is the only place a reader is told which ones. Differences: +$(diff <(printf '%s\n' "$disabled") <(printf '%s\n' "$recorded") \ + | sed 's/^< / only in Checks: /; s/^> / only in header-reach: /' \ + | grep -E '^ only' || true)" +else + note "the \`header-reach:\` list names all ${recorded_n} subtracted check(s)" +fi + +# -- The behavioural probe --------------------------------------------------- +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +mkdir -p "${scratch}/include/morph" "${scratch}/tests" "${scratch}/src" +cp "$root_config" "${scratch}/.clang-tidy" +cp "$tests_config" "${scratch}/tests/.clang-tidy" + +# One finding per probed check, both inside the header, both in ordinary +# non-template inline functions so no instantiation is needed to reach them. +cat > "${scratch}/include/morph/probe_scope.hpp" <<'HPP' +#pragma once +#include + +inline int probeUncheckedContainerAccess(const std::vector& values) +{ + return values[0]; +} + +inline int probeShortIdentifier(const std::vector& values) +{ + const int ab = static_cast(values.size()); + return ab; +} +HPP + +cat > "${scratch}/tests/probe_scope_tu.cpp" <<'CPP' +#include "../include/morph/probe_scope.hpp" + +int probeScopeFromTests(const std::vector& values) +{ + return probeUncheckedContainerAccess(values) + probeShortIdentifier(values); +} +CPP +cp "${scratch}/tests/probe_scope_tu.cpp" "${scratch}/src/probe_scope_tu.cpp" + +run_probe() { + # clang-tidy exits non-zero on findings (WarningsAsErrors: "*"), which is + # the normal case here, so its status is deliberately ignored: what the + # assertions read is which check names appear against the probe header. + ( cd "$scratch" && "$tidy" --quiet "$1" -- -std=c++23 2>&1 ) || true +} + +from_tests="$(run_probe tests/probe_scope_tu.cpp)" +from_src="$(run_probe src/probe_scope_tu.cpp)" + +for check in "${probe_checks[@]}"; do + # The control first: if the probe does not fire from a TU outside tests/, + # nothing below means anything. + if printf '%s' "$from_src" | grep -q "probe_scope\.hpp.*\[${check}"; then + note "probe: ${check} is reported in the header from src/probe_scope_tu.cpp" + else + fail "probe control failed: ${check} was NOT reported against + include/morph/probe_scope.hpp from a TU outside tests/. The probe, not the + repository, is broken -- and until it is fixed the assertion below proves + nothing. Full output: +$(printf '%s' "$from_src" | sed 's/^/ /')" + continue + fi + + if printf '%s' "$from_tests" | grep -q "probe_scope\.hpp.*\[${check}"; then + fail "${check} IS now reported against include/morph/probe_scope.hpp from a + TU under tests/, where tests/.clang-tidy subtracts it. clang-tidy appears to + resolve configuration per diagnostic file rather than per translation unit, + which is the opposite of what morph#632 measured and of what the note in + tests/.clang-tidy tells readers. Re-measure the reach, correct or delete + that note, and revisit any header gate built on the old behaviour." + else + note "probe: ${check} is suppressed in the header from tests/probe_scope_tu.cpp" + fi +done + +if [ "$failures" -ne 0 ]; then + printf '\n%s clang-tidy suppression-scope check(s) failed\n' "$failures" >&2 + exit 1 +fi + +printf '\nok: tests/.clang-tidy records the reach it actually has\n' diff --git a/scripts/test_check_tidy_suppression_scope.sh b/scripts/test_check_tidy_suppression_scope.sh new file mode 100755 index 000000000..d7822fe04 --- /dev/null +++ b/scripts/test_check_tidy_suppression_scope.sh @@ -0,0 +1,167 @@ +#!/usr/bin/env bash +# Usage: bash scripts/test_check_tidy_suppression_scope.sh [CLANG_TIDY_BINARY] +# +# Self-test for scripts/check_tidy_suppression_scope.sh, the gate that keeps +# tests/.clang-tidy's record of its own reach true (morph#632). +# +# A lint gate nobody tests reports green whether or not it still detects +# anything. This one is exposed to that twice over: the record it checks is +# written correct in the same commit that adds the gate, and its behavioural +# half asserts a *negative* -- that a check does not fire -- which is the +# single easiest assertion in this repository to satisfy by accident. A probe +# that failed to compile, a check name misspelt, a clang-tidy invocation that +# analysed nothing: each produces "no finding" and each would score as a pass. +# +# So the drifts the gate claims to catch are reintroduced into a scratch copy, +# one at a time, and must be caught for the stated reason -- including the +# broken-probe case, where the gate must fail rather than report suppression. +# +# Needs a clang-tidy binary. Each case runs the real gate against a scratch +# tree, so the wall clock is a few clang-tidy invocations per case. +set -euo pipefail + +readonly tidy="${1:-clang-tidy}" +readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +readonly checker="scripts/check_tidy_suppression_scope.sh" + +failures=0 + +note() { printf 'ok: %s\n' "$*"; } +fail() { printf 'error: %s\n' "$*" >&2; failures=$((failures + 1)); } + +scratch="$(mktemp -d)" +trap 'rm -rf "$scratch"' EXIT + +# The gate reads .clang-tidy, tests/.clang-tidy and its own source; a copy of +# those three paths is the whole tree it needs. +readonly pristine="${scratch}/pristine" +mkdir -p "${pristine}/tests" "${pristine}/scripts" +cp "${repo_root}/.clang-tidy" "${pristine}/.clang-tidy" +cp "${repo_root}/tests/.clang-tidy" "${pristine}/tests/.clang-tidy" +cp "${repo_root}/${checker}" "${pristine}/scripts/" + +make_tree() { + local dest="$1" + rm -rf "$dest" + mkdir -p "$dest" + cp -R "${pristine}/." "$dest" +} + +# `sed -i` is not portable between GNU and BSD sed; edit through a temp file. +edit() { + local file="$1"; shift + sed "$@" "$file" > "${file}.new" + mv "${file}.new" "$file" +} + +expect_caught() { + local description="$1" mutator="$2" expected="$3" + local tree="${scratch}/case" output + make_tree "$tree" + if ! ( cd "$tree" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "$tree" && bash "$checker" "$tidy" . 2>&1 )"; then + fail "NOT caught: ${description} -- the gate passed a tree it should reject" + printf '%s\n' "$output" >&2 + return + fi + if printf '%s' "$output" | grep -qF "$expected"; then + note "caught: ${description}" + else + fail "caught for the WRONG reason: ${description} -- no diagnostic containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +expect_accepted() { + local description="$1" mutator="$2" + local tree="${scratch}/case" output + make_tree "$tree" + if ! ( cd "$tree" && eval "$mutator" ); then + fail "mutator failed to apply: ${description}" + return + fi + if output="$( cd "$tree" && bash "$checker" "$tidy" . 2>&1 )"; then + note "accepted: ${description}" + else + fail "FALSE POSITIVE: ${description} -- the gate rejected a tree it should accept:" + printf '%s\n' "$output" >&2 + fi +} + +# -- The unmodified tree must pass ------------------------------------------- +make_tree "${scratch}/clean" +if output="$( cd "${scratch}/clean" && bash "$checker" "$tidy" . 2>&1 )"; then + note "the unmodified tree passes" +else + fail "the unmodified tree was rejected by the gate:" + printf '%s\n' "$output" >&2 +fi + +# -- The record going stale -------------------------------------------------- +# A fourteenth suppression added without extending the record. This is how the +# note in tests/.clang-tidy stops being true, and it is the drift with the +# highest chance of actually happening. +expect_caught "a suppression added without a header-reach: entry" \ + "edit tests/.clang-tidy -e 's|^ -bugprone-chained-comparison,| -bugprone-chained-comparison,\n -performance-unnecessary-value-param,|'" \ + "only in Checks: performance-unnecessary-value-param" + +# The mirror: a record that claims more than the file suppresses is just as +# wrong, and tells a reader a check is unreportable when it is not. +expect_caught "a header-reach: entry for a check that is not suppressed" \ + "edit tests/.clang-tidy -e 's|^# header-reach: bugprone-chained-comparison|# header-reach: bugprone-chained-comparison\n# header-reach: misc-const-correctness|'" \ + "only in header-reach: misc-const-correctness" + +# A suppression removed from Checks: but left in the record. +expect_caught "a suppression removed but left in the record" \ + "edit tests/.clang-tidy -e '/^ -readability-identifier-length,$/d'" \ + "only in header-reach: readability-identifier-length" + +# -- The gate must not go blind ---------------------------------------------- +# If the Checks: block is reworded out from under the parser, the honest +# answer is failure. Both lists would read as empty and match each other -- +# the exact shape of "a control that reports success while measuring nothing". +expect_caught "the Checks: block becomes unparseable" \ + "edit tests/.clang-tidy -e 's|^Checks: >|Disabled: >|'" \ + "subtracts no checks at all" + +# -- The behavioural half ---------------------------------------------------- +# The mechanism reversing: the suppression stops reaching headers. Simulated +# by taking the probed check out of tests/.clang-tidy's Checks: while leaving +# the record intact -- from the probe's point of view that is exactly what +# per-diagnostic-file config resolution would look like. The list mismatch is +# caught too; the assertion below is that the *behavioural* diagnostic fires, +# which is what tells a maintainer the note is now wrong rather than untidy. +expect_caught "the probed check stops being suppressed in headers" \ + "edit tests/.clang-tidy -e '/^ -cppcoreguidelines-pro-bounds-avoid-unchecked-container-access,$/d'" \ + "resolve configuration per diagnostic file" + +# The anti-vacuity control, and the reason this self-test exists. Break the +# probe's ability to produce a finding at all -- here by disabling the probed +# checks at the repository root, so they fire from nowhere -- and the gate must +# report a broken probe. A gate that instead said "suppressed in the header" +# would be asserting a negative it never measured. +expect_caught "the probe can no longer fire from anywhere" \ + "edit .clang-tidy -e 's|^ -misc-include-cleaner| -misc-include-cleaner,\n -cppcoreguidelines-pro-bounds-avoid-unchecked-container-access,\n -readability-identifier-length|'" \ + "probe control failed" + +# -- False positives --------------------------------------------------------- +# Rewording the prose around the record is not drift. Only the machine-readable +# list and the behaviour are the gate's business. +expect_accepted "the surrounding prose rewritten" \ + "edit tests/.clang-tidy -e 's|^# Measured, not inferred\..*|# Some other wording entirely.|'" + +# Adding a suppression *and* its record entry together: the ordinary way the +# file grows. If this failed, the gate would be a tax on maintaining it. +expect_accepted "a suppression added together with its header-reach: entry" \ + "edit tests/.clang-tidy -e 's|^ -bugprone-chained-comparison,| -bugprone-chained-comparison,\n -performance-unnecessary-value-param,|' \ + && edit tests/.clang-tidy -e 's|^# header-reach: bugprone-chained-comparison|# header-reach: bugprone-chained-comparison\n# header-reach: performance-unnecessary-value-param|'" + +if [ "$failures" -ne 0 ]; then + printf '\n%d case(s) failed.\n' "$failures" >&2 + exit 1 +fi + +printf '\nall cases passed.\n' diff --git a/tests/.clang-tidy b/tests/.clang-tidy index 1da606b9a..3692cebcd 100644 --- a/tests/.clang-tidy +++ b/tests/.clang-tidy @@ -1,12 +1,92 @@ -# Checks suppressed inside tests/ only. Every entry below is a check whose -# finding is *test idiom* — a property of Catch2 or of a raw-syscall harness — -# rather than a defect. Checks whose findings turned out to be real problems -# were fixed in the test sources instead of being listed here; in particular -# misc-const-correctness, readability-container-contains, +# Checks suppressed for translation units under tests/. Every entry below is a +# check whose finding is *test idiom* — a property of Catch2 or of a raw-syscall +# harness — rather than a defect. Checks whose findings turned out to be real +# problems were fixed in the test sources instead of being listed here; in +# particular misc-const-correctness, readability-container-contains, # cppcoreguidelines-special-member-functions, performance-unnecessary-value-param # and bugprone-empty-catch are deliberately still enabled here. # # One entry per check, with the reason it cannot fire on anything worth fixing. +# +# ── These suppressions are NOT confined to test sources (morph#632) ────────── +# +# clang-tidy resolves its configuration from the path of the *translation unit* +# it is analysing, not from the path of the file a diagnostic lands in. This +# file's directory therefore scopes which TUs it governs; it does not scope +# which files their diagnostics may name. Every entry below is also off for +# every header those TUs reach — all of include/morph/** included — so the same +# finding in the same header is reported from an examples/ TU and silently +# dropped from a tests/ one. +# +# Each entry's reasoning below is a correct statement about test code and says +# nothing about include/morph/. The reasoning is not the problem; its unstated +# reach is. +# +# Measured, not inferred. On a8511aa6 with clang-tidy 22.1.8, over the 134 +# tests/ translation units in the compile database the clang-tidy-diff job +# configures for itself (every optional feature ON), every TU analysed and none +# failing to compile: +# +# distinct findings inside include/morph/**, from those 134 TUs +# with this file in place .... 262 +# with this file removed ..... 595 +# hidden by this file ........ 333, across 25 headers +# +# 247 cppcoreguidelines-pro-bounds-avoid-unchecked-container-access +# 57 readability-identifier-length +# 6 cppcoreguidelines-pro-bounds-array-to-pointer-decay +# 5 modernize-avoid-c-arrays +# 5 cppcoreguidelines-avoid-c-arrays +# 5 bugprone-unchecked-optional-access +# 3 cppcoreguidelines-pro-type-vararg +# 3 readability-convert-member-functions-to-static +# 2 readability-function-cognitive-complexity +# +# most affected: forms/forms.hpp 90, session/session_auth.hpp 61, +# net/detail/sha1.hpp 32, forms/views.hpp 31, util/quantity.hpp 19 +# +# The consequence for anyone building a gate over include/morph/**: a gate +# driven by TUs under tests/ cannot report any of the checks listed under +# `header-reach:` below, in any header it reaches, and would go green on +# findings it was built to catch. Drive it from TUs whose path is not under +# tests/ — the verify_interface_header_sets stubs generated under build/ sit +# outside this file's reach and inherit the repository-root configuration — or +# record, wherever the gate is documented, which checks it is structurally +# unable to report. +# +# This is a property of the tool, not a choice: clang-tidy offers no way to +# scope a suppression to the file a diagnostic lands in rather than to the TU. +# scripts/check_tidy_suppression_scope.sh pins both halves of the record — that +# the reach still exists, behaviourally, so this note goes red rather than +# quietly stale if that ever changes, and that the list below still names +# exactly what `Checks:` subtracts. +# +# header-reach: bugprone-chained-comparison +# header-reach: bugprone-misplaced-widening-cast +# header-reach: bugprone-unchecked-optional-access +# header-reach: cppcoreguidelines-avoid-c-arrays +# header-reach: cppcoreguidelines-pro-bounds-array-to-pointer-decay +# header-reach: cppcoreguidelines-pro-bounds-avoid-unchecked-container-access +# header-reach: cppcoreguidelines-pro-type-vararg +# header-reach: misc-use-internal-linkage +# header-reach: modernize-avoid-c-arrays +# header-reach: readability-convert-member-functions-to-static +# header-reach: readability-function-cognitive-complexity +# header-reach: readability-identifier-length +# header-reach: readability-named-parameter +# +# The nine examples/*/tests/.clang-tidy and examples/common/testkit/.clang-tidy +# copies have the same reach and, measured on the same revision, no effect +# through it: they subtract exactly one check, bugprone-chained-comparison, +# which fires only on Catch2's own REQUIRE expansion and is attributed to the +# test source line rather than to any morph header. Over the 154 example test +# TUs in the same database, findings inside include/morph/** were identical +# with and without those nine files — 312 either way. That measurement cannot +# by itself separate "no header reach" from "the check never fired": Catch2 +# 3.16.0 locally carries the NOLINT that suppresses it, and this check produced +# zero findings in either run. The structural argument above is what carries +# the conclusion; the measurement only fails to contradict it. +# ───────────────────────────────────────────────────────────────────────────── # Catch2's REQUIRE(a == b) macro expansion is parsed by clang-tidy as a # chained comparison. This is a false positive — not a real code issue.