From 4f7d8923b556b1fbc27d2d2319f3d029a163d4b7 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 10:33:11 +0200 Subject: [PATCH 1/2] ci: build MORPH_BUILD_BANK_GUI, and derive the option list rather than assert it (fixes #605) `MORPH_BUILD_BANK_GUI` defaults OFF and was set ON only by the `linux-everything` preset in CMakePresets.json, which no workflow names. The native `bank_gui` / `bank_gui_qml_tests` targets were therefore compiled by no job, which is how #604 happened: a target went unbuildable on master with every gate green, and the job called "Linux / all optional features" went on passing, because its option list is hand-written and nothing compared it to the options that exist. Two parts. (a) Add `-DMORPH_BUILD_BANK_GUI=ON` and `-DMORPH_BUILD_BANK_EXAMPLE=ON` (which it needs -- the root CMakeLists only descends into examples/bank under the latter, and examples/bank only into gui/ under the former) to the `linux-all-features` configure step. That is the one leg that can host them: it already installs Qt 6.8.1 from aqtinstall, which the GUI's Qml/Quick/QuickControls2 at 6.5+ requires and the distro Qt the other bank jobs use cannot give, and already installs the ODBC/SQLite/yaml-cpp/libzip set the bank example's Lightweight fetch needs. (b) A gate that derives the list. Adding the flag closes the hole once; it does not close the class, and AGENTS.md's own test -- "ask whether the check would still pass if the feature did nothing" -- answers yes for (a) alone. Nothing stops option 17 from being declared next month and enabled by nobody. scripts/check_workflow_option_coverage.py enumerates every `option(MORPH_BUILD_...)` in the tracked CMake files and every `-DMORPH_BUILD_*=ON` in .github/workflows/, and fails when a declared option is built by nothing. Covered means: declared default ON, or enabled by a native workflow, or enabled through a matrix key some leg sets ON, or exempt with a written reason. Its failure message says how to cover the option, not how to exempt it. Two corrections to the issue's accounting, both found by writing the check: - `MORPH_BUILD_DOCUMENTATION` is *not* exempt. The issue put it in the exemption set beside `CLANG_TIDY`, but docs.yml's configure step passes `-DMORPH_BUILD_DOCUMENTATION=ON` literally, so it is covered by derivation. Exempting it would have been exactly the hand-maintained falsehood this gate exists to prevent, so the checker also rejects an exemption for an option that is in fact covered, and one naming an option no CMake file declares. `MORPH_BUILD_CLANG_TIDY` is the only exemption. - `MORPH_BUILD_BANK_GUI` was not enabled by *nothing*: wasm-demo.yml has passed `-DMORPH_BUILD_BANK_GUI=ON` all along. Under EMSCRIPTEN, examples/bank/CMakeLists.txt descends into gui_wasm/ and returns before the native gui/ exists, so the same option name selects a disjoint subtree and that build proves nothing about the target #604 found broken. Counting it would have made this gate vacuous for the one option it was written for, so Emscripten workflows are classified apart -- derived from the workflow text (`emcmake`/`emsdk`/`qt-cmake`), not from a list of filenames. The checker is not trusted on its own report: scripts/test_check_workflow_- option_coverage.sh reintroduces each drift it claims to catch into a scratch copy of the tree, one at a time, and requires it to be caught for the stated reason -- including dropping (a)'s own flag, which must go red despite wasm-demo.yml. Twelve cases, nine of them mutations and three false-positive mirrors. The `option-coverage` job runs the self-test before the check, the same order the deprecation, test-type-name and journal-stamp lints use. Its home is drift-guard.yml once #614 lands; that file is held, so it is here. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- .github/workflows/ci.yml | 50 ++++ scripts/check_workflow_option_coverage.py | 251 ++++++++++++++++++ .../test_check_workflow_option_coverage.sh | 198 ++++++++++++++ 3 files changed, 499 insertions(+) create mode 100644 scripts/check_workflow_option_coverage.py create mode 100755 scripts/test_check_workflow_option_coverage.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8c710637..f608e297 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1803,6 +1803,20 @@ jobs: # engine-load smoke test were built by nothing at all. This job has # Qt ${{ env.QT_VERSION }} from aqtinstall, so here they are built, # and the smoke test runs, on every push. + # + # MORPH_BUILD_BANK_GUI (with MORPH_BUILD_BANK_EXAMPLE, which it needs + # -- examples/bank/CMakeLists.txt only descends into gui/ from the + # bank example's own subdirectory) was, until this line, the single + # MORPH_BUILD_* option no job in .github/workflows/ enabled at all: + # the `linux-everything` preset that sets it is named by no workflow. + # morph#604 is what that costs -- bank_gui stopped compiling on + # master with every gate green. This is the one leg that can host it: + # it already installs Qt ${{ env.QT_VERSION }} from aqtinstall (the + # GUI needs Qml/Quick/QuickControls2 at 6.5+, which the distro Qt the + # other bank jobs use cannot give) and already installs the + # ODBC/SQLite/yaml-cpp/libzip set the bank example's Lightweight + # fetch requires. The check-workflow-option-coverage job below is + # what keeps the next option from repeating this. # No -DCMAKE_..._COMPILER_LAUNCHER=sccache: see linux-compilers' # Configure steps for why leaving it unset is what lets # fastcache-cc be selected. @@ -1816,6 +1830,8 @@ jobs: -DMORPH_BUILD_LOAD_TESTS=ON \ -DMORPH_BUILD_HMAC_EXAMPLES=ON \ -DMORPH_BUILD_FUZZERS=${{ matrix.fuzzers }} \ + -DMORPH_BUILD_BANK_EXAMPLE=ON \ + -DMORPH_BUILD_BANK_GUI=ON \ -DMORPH_REQUIRE_VETTED_HMAC=ON \ $EXTRA \ $MORPH_FASTCACHE_AUTO_INSTALL_FLAG @@ -2364,6 +2380,40 @@ jobs: - name: Check hand-rolled LogEntry constructions stamp schema run: bash scripts/check_journal_stamps.sh examples + # ── Every MORPH_BUILD_* option is built by some job ──────────────────── + # + # Its own job, and deliberately a cheap one: it reads CMake files and + # 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 #614 lands; it is held there now, so the step lives here.) + # + # Catches morph#605's bug class: MORPH_BUILD_BANK_GUI was set ON by a + # CMakePresets.json preset no workflow ever names, so it was compiled by no + # native job at all -- and bank_gui duly stopped compiling on master with + # every gate green (morph#604), while the job named "all optional features" + # went on passing. Adding the missing flag closes that hole once; only a + # check that *derives* the option list closes the class, because nothing + # stops the next option from being declared and enabled by nobody. + option-coverage: + name: Every MORPH_BUILD_* option is built by some job + runs-on: ubuntu-24.04 + steps: + - uses: actions/checkout@v4 + + # See scripts/test_check_workflow_option_coverage.sh's own comment: a + # lint gate nobody tests reports green whether or not it still detects + # anything, and this one is maximally exposed to that -- the tree it + # guards is correct the day it lands, so it passes on day one whether it + # parses anything at all. Its whole value is what it does on a day that + # has not happened yet. + - name: Self-test the option-coverage checker + run: bash scripts/test_check_workflow_option_coverage.sh + + - name: Check every declared MORPH_BUILD_* option is enabled by some job + run: python3 scripts/check_workflow_option_coverage.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_option_coverage.py b/scripts/check_workflow_option_coverage.py new file mode 100644 index 00000000..328c79c4 --- /dev/null +++ b/scripts/check_workflow_option_coverage.py @@ -0,0 +1,251 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: Apache-2.0 +"""Usage: python3 scripts/check_workflow_option_coverage.py [REPO_ROOT] + +Fails if any `option(MORPH_BUILD_...)` declared in this tree is built by no job +in .github/workflows/. + +Why this gate exists (morph#605, morph#604): `MORPH_BUILD_BANK_GUI` defaulted +OFF, was set ON only by a `CMakePresets.json` preset (`linux-everything`) that +no workflow ever names, and was therefore compiled by nothing in CI. bank_gui +duly stopped compiling on master with every gate green (morph#604), and the job +called "Linux / all optional features" went on passing, because its option list +is hand-written and nothing compared it to the options that exist. + +Adding the missing flag closes that hole once. It does not close the *class*: +nothing stops the next `option(MORPH_BUILD_...)` from being declared next month +and enabled by nobody, at which point this issue gets filed a third time. So +the check derives both sides -- the declared options from the CMake files, the +enabled ones from the workflows -- rather than asserting a list. It goes red +when a new option appears uncovered, which a hand-maintained list cannot do. + +What counts as covered, in order: + + 1. The option is declared with default ON. It is then built by every job that + does not explicitly turn it off, so no workflow has to name it. + 2. Some *native* workflow passes `-DMORPH_BUILD_=ON` outside a comment. + 3. Some native workflow passes `-DMORPH_BUILD_=${{ matrix. }}` and + some matrix entry in that same file sets `` to ON. This is how + MORPH_BUILD_FUZZERS is covered (clang only -- libFuzzer needs it), and + resolving it rather than special-casing the name means a matrix that + quietly went all-OFF would fail here. + 4. The option is in EXEMPT below, with a written reason. + +Two things this deliberately does not count. + +Comment lines are stripped before scanning, so a flag *discussed* in a comment +never counts as coverage. That is not hypothetical: ci.yml's own configure-step +comments mention `-DMORPH_BUILD_TESTS=ON` and `MORPH_BUILD_FORMS_QML=ON` in +prose. + +An Emscripten workflow does not count either, and MORPH_BUILD_BANK_GUI is the +reason the distinction had to be drawn: wasm-demo.yml has passed +`-DMORPH_BUILD_BANK_GUI=ON` all along, yet the target morph#604 found broken +was the *native* one. Under EMSCRIPTEN, examples/bank/CMakeLists.txt descends +into `gui_wasm/` and returns before the native `gui/` subdirectory exists, so +the same option name selects a disjoint subtree. Counting that as coverage +would have made this whole gate vacuous for the one option it was written for. +A workflow is treated as Emscripten when its text invokes `emcmake`, `emsdk` or +`qt-cmake` -- derived from the workflow, so a new WASM workflow is classified +by being written rather than by being added to a list here. + +Two rules keep EXEMPT from becoming the stale list this gate exists to replace: +an entry for an option that is not declared anywhere is an error, and an entry +for an option that *is* covered is an error. An exemption has to be necessary +to be allowed to stay. + +Finding zero declared options is a failure, not a pass -- a gate with nothing +left to check reports green exactly as loudly as one that checked everything. +""" + +from __future__ import annotations + +import re +import subprocess +import sys +from pathlib import Path + +# Declared options that no job builds, and should not. The reason is the +# payload: an exemption without one is the hand-maintained record this gate +# replaces. Removing an option from here is always allowed; adding one needs an +# argument that a reviewer can check. +EXEMPT: dict[str, str] = { + # Not a build feature: setting it makes every compile in the tree run + # clang-tidy with warnings-as-errors. ci.yml's `clang-tidy` job covers the + # same ground by running clang-tidy-diff.py over the pull request's diff + # instead, which is both faster and scoped to changed lines -- turning the + # CMake option on in some other job would re-tidy the whole tree on every + # push to no additional effect. + # + # MORPH_BUILD_DOCUMENTATION is deliberately *not* here, though it also has + # a dedicated job: docs.yml's configure step passes + # -DMORPH_BUILD_DOCUMENTATION=ON literally, so it is covered by rule 2 and + # an exemption for it would be a false record. + "MORPH_BUILD_CLANG_TIDY": "covered by ci.yml's `clang-tidy` job, which runs " + "clang-tidy-diff.py over the PR diff rather than through the CMake option", +} + +OPTION_RE = re.compile( + r"""^[ \t]*option[ \t]*\([ \t]*(MORPH_BUILD_[A-Z0-9_]+)[ \t]+"[^"]*"[ \t]+(ON|OFF)[ \t]*\)""", + re.MULTILINE, +) +# The value alternative takes `${{ ... }}` first: an Actions expression carries +# spaces inside its braces, so a plain `\S+` would capture the literal `${{` +# and every matrix-valued flag would silently read as unresolvable. +FLAG_RE = re.compile(r"-D(MORPH_BUILD_[A-Z0-9_]+)=(\$\{\{[^}]*\}\}|\S+)") +MATRIX_EXPR_RE = re.compile(r"^\$\{\{\s*matrix\.([A-Za-z0-9_-]+)\s*\}\}$") +EMSCRIPTEN_RE = re.compile(r"\bemcmake\b|\bemsdk\b|qt-cmake") + + +def tracked_cmake_files(root: Path) -> list[Path]: + """Every tracked CMakeLists.txt / *.cmake, so an option declared in a + subdirectory counts too (examples/vetted_hmac/CMakeLists.txt declares two).""" + out = subprocess.run( + [ + "git", + "-C", + str(root), + "ls-files", + "-z", + "CMakeLists.txt", + "*/CMakeLists.txt", + "*.cmake", + ], + capture_output=True, + text=True, + check=True, + ).stdout + return [root / name for name in out.split("\0") if name] + + +def strip_comments(text: str) -> str: + """Drop whole-line comments. Covers both YAML comments and the shell + comments inside `run: |` blocks, which is where ci.yml discusses flags it + does not pass.""" + return "\n".join(line for line in text.splitlines() if not line.lstrip().startswith("#")) + + +def declared_options(root: Path) -> dict[str, str]: + """Option name -> declared default ('ON' or 'OFF').""" + found: dict[str, str] = {} + for path in tracked_cmake_files(root): + for name, default in OPTION_RE.findall(path.read_text(encoding="utf-8")): + found[name] = default + return found + + +def matrix_key_can_be_on(body: str, key: str) -> bool: + """True when some matrix entry in this workflow sets `key` to ON.""" + return re.search(rf"^\s*{re.escape(key)}:\s*['\"]?ON['\"]?\s*$", body, re.MULTILINE) is not None + + +def enabled_options(root: Path) -> tuple[dict[str, list[str]], dict[str, list[str]]]: + """(native, wasm-only): option name -> the workflow files that enable it. + + Kept apart rather than merged because an Emscripten configure of the same + option name can compile an entirely different subtree -- see the module + docstring. Only the first return value counts as coverage; the second is + reported so an option that is enabled *only* under WASM says so in the + failure output instead of looking like it was never enabled at all.""" + native: dict[str, list[str]] = {} + wasm: dict[str, list[str]] = {} + for path in sorted((root / ".github" / "workflows").glob("*.yml")): + raw_text = path.read_text(encoding="utf-8") + body = strip_comments(raw_text) + bucket = wasm if EMSCRIPTEN_RE.search(body) else native + for name, value in FLAG_RE.findall(body): + if value.rstrip("\\,;:)\"'") == "ON": + bucket.setdefault(name, []).append(path.name) + continue + expr = MATRIX_EXPR_RE.match(value) + if expr and matrix_key_can_be_on(body, expr.group(1)): + bucket.setdefault(name, []).append(path.name) + return native, wasm + + +def main(argv: list[str]) -> int: + root = Path(argv[1] if len(argv) > 1 else ".").resolve() + + declared = declared_options(root) + if not declared: + print( + "::error::found no option(MORPH_BUILD_...) declarations at all. Either " + "the declaration syntax changed or this checker is scanning the wrong " + "tree; a coverage gate with nothing left to check is not a passing gate.", + file=sys.stderr, + ) + return 1 + + enabled, wasm_only = enabled_options(root) + errors: list[str] = [] + + for name in sorted(declared): + if declared[name] == "ON" or name in enabled or name in EXEMPT: + continue + note = "" + if name in wasm_only: + note = ( + f" It is enabled by {', '.join(sorted(set(wasm_only[name])))}, but " + f"that is an Emscripten build: the same option can select a " + f"different subtree there (examples/bank/CMakeLists.txt builds " + f"gui_wasm/ and returns), so it does not prove the native code " + f"compiles." + ) + errors.append( + f"{name} is declared in this tree, defaults OFF, and no native job in " + f".github/workflows/ passes -D{name}=ON -- so nothing in CI ever " + f"compiles the code it guards.{note} " + f"Cover it: add -D{name}=ON to the configure step of a job whose " + f"runner already has what it needs. 'Linux / all optional features' " + f"in ci.yml is the leg meant to carry these, and already installs Qt " + f"from aqtinstall plus the ODBC/SQLite/yaml-cpp/libzip set. " + f"A CMakePresets.json preset that sets the option is not coverage -- " + f"a preset no workflow names is exactly how morph#605 happened." + ) + + for name, reason in sorted(EXEMPT.items()): + if name not in declared: + errors.append( + f"{name} is exempted by this checker but is declared by no CMake " + f"file in the tree. Delete the exemption: it is a stale record of " + f"an option that no longer exists." + ) + elif name in enabled: + errors.append( + f"{name} is exempted by this checker (reason: {reason}) but is in " + f"fact enabled by {', '.join(sorted(set(enabled[name])))}. Delete " + f"the exemption -- one that is not doing anything is cover for the " + f"next one that is wrong." + ) + elif declared.get(name) == "ON": + errors.append( + f"{name} is exempted by this checker but is declared with default " + f"ON, so it is already built everywhere. Delete the exemption." + ) + + if errors: + for err in errors: + print(f"::error::{err}", file=sys.stderr) + print( + f"\n{len(errors)} problem(s). {len(declared)} MORPH_BUILD_* option(s) " + f"declared, {len(enabled)} enabled by a workflow, {len(EXEMPT)} exempt.", + file=sys.stderr, + ) + return 1 + + print(f"{len(declared)} MORPH_BUILD_* option(s) declared, all accounted for:") + for name in sorted(declared): + if name in enabled: + where = "enabled by " + ", ".join(sorted(set(enabled[name]))) + elif declared[name] == "ON": + where = "declared default ON (built unless a job turns it off)" + else: + where = f"exempt -- {EXEMPT[name]}" + if name in wasm_only: + where += " [also under WASM: " + ", ".join(sorted(set(wasm_only[name]))) + "]" + print(f" {name}: {where}") + return 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv)) diff --git a/scripts/test_check_workflow_option_coverage.sh b/scripts/test_check_workflow_option_coverage.sh new file mode 100755 index 00000000..f1629ed8 --- /dev/null +++ b/scripts/test_check_workflow_option_coverage.sh @@ -0,0 +1,198 @@ +#!/usr/bin/env bash +# Usage: bash scripts/test_check_workflow_option_coverage.sh +# +# Self-test for scripts/check_workflow_option_coverage.py, the gate that keeps +# every declared `option(MORPH_BUILD_...)` built by some job in +# .github/workflows/. +# +# A lint gate nobody tests reports green whether or not it still detects +# anything, and this one is maximally exposed to that: the tree it guards is +# correct the moment the gate lands, so it passes on day one whether it is +# parsing anything at all. Its whole value is what it does on a day that has +# not happened yet -- the day someone declares option 17 and enables it +# nowhere. So the gate is checked in both directions: the unmodified tree must +# pass, and every drift it 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. +# +# The checker enumerates CMake files with `git ls-files`, so each case runs +# against a throwaway git repository holding a copy of this one's tracked files +# rather than against a plain directory. +set -euo pipefail + +readonly repo_root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" +readonly checker="scripts/check_workflow_option_coverage.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 + +readonly pristine="${scratch}/pristine" +mkdir -p "$pristine" +while IFS= read -r -d '' tracked; do + mkdir -p "${pristine}/$(dirname "$tracked")" + cp "${repo_root}/${tracked}" "${pristine}/${tracked}" +done < <(cd "$repo_root" && git ls-files -z) +git -C "$pristine" init -q +git -C "$pristine" add -A + +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" +} + +# 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 ----------------------------------------- +# The next MORPH_BUILD_* option, declared and enabled by nobody. This is the +# case morph#605 is about: not the option that was missing when the gate was +# written, but the one that will be missing after it. +expect_caught "a newly declared option that no job enables" \ + "printf '%s\n' 'option(MORPH_BUILD_TELEMETRY \"Build the telemetry sink\" OFF)' >> CMakeLists.txt" \ + "MORPH_BUILD_TELEMETRY is declared in this tree, defaults OFF" + +# The original hole, reintroduced: drop the flag morph#605 added and the gate +# must go red again. This is the case that proves the gate is not vacuous for +# the very option that motivated it. +expect_caught "MORPH_BUILD_BANK_GUI dropped from ci.yml's all-features job" \ + "edit .github/workflows/ci.yml -e '/^ *-DMORPH_BUILD_BANK_GUI=ON /d'" \ + "MORPH_BUILD_BANK_GUI is declared in this tree, defaults OFF" + +# ...and the WASM enablement must not paper over it. wasm-demo.yml has passed +# -DMORPH_BUILD_BANK_GUI=ON all along; under EMSCRIPTEN that builds gui_wasm/ +# and never the native gui/ target morph#604 found broken. If this assertion +# ever stops holding, the gate has gone blind to its own founding case. +expect_caught "the Emscripten enablement does not count as native coverage" \ + "edit .github/workflows/ci.yml -e '/^ *-DMORPH_BUILD_BANK_GUI=ON /d'" \ + "but that is an Emscripten build" + +# -- What must not count as coverage ----------------------------------------- +# A flag mentioned in a comment. ci.yml's configure steps discuss flags they do +# not pass, so a checker that grepped the raw text would score them as covered. +expect_caught "the only enablement is commented out" \ + "edit .github/workflows/ci.yml -e 's|^\( *\)-DMORPH_BUILD_LOAD_TESTS=ON |\1# -DMORPH_BUILD_LOAD_TESTS=ON |'" \ + "MORPH_BUILD_LOAD_TESTS" + +# A matrix-valued flag whose matrix has quietly gone all-OFF. The flag line is +# untouched and still reads `-DMORPH_BUILD_FUZZERS=${{ matrix.fuzzers }}`; only +# the values behind it change. A checker that accepted the expression on sight +# would report this covered. The literal `-DMORPH_BUILD_FUZZERS=ON` in the +# clang-tidy job goes too, or it would cover the option by itself and this case +# would prove nothing about the matrix resolution. +expect_caught "every matrix leg sets the fuzzers key OFF" \ + "edit .github/workflows/ci.yml -e '/-DMORPH_BUILD_FUZZERS=ON/d' \ + && edit .github/workflows/ci.yml -e \"s/^\( *\)fuzzers: 'ON'/\1fuzzers: 'OFF'/\"" \ + "MORPH_BUILD_FUZZERS" + +# The other half of the same rule: with the literal gone, an ON matrix leg +# still counts. Without this case the rule above would be satisfied by a +# checker that simply never resolved a matrix expression at all. +expect_accepted "a matrix leg setting the key ON is the only enablement" \ + "edit .github/workflows/ci.yml -e '/-DMORPH_BUILD_FUZZERS=ON/d'" + +# -- The exemption set must stay necessary ----------------------------------- +# An exemption for an option that is in fact enabled. This is the shape the +# gate replaces: a hand-written record that is no longer true and that nothing +# reads back against reality. +expect_caught "an exemption for an option some job does enable" \ + "edit ${checker} -e 's|^ \"MORPH_BUILD_CLANG_TIDY\": | \"MORPH_BUILD_NET\": \"stale\",\n \"MORPH_BUILD_CLANG_TIDY\": |'" \ + "MORPH_BUILD_NET is exempted by this checker" + +# An exemption for an option that no longer exists. +expect_caught "an exemption naming an option no CMake file declares" \ + "edit ${checker} -e 's|^ \"MORPH_BUILD_CLANG_TIDY\": | \"MORPH_BUILD_GONE\": \"stale\",\n \"MORPH_BUILD_CLANG_TIDY\": |'" \ + "is declared by no CMake file in the tree" + +# -- The gate must not go blind ---------------------------------------------- +# If the declaration syntax moves out from under the parser, the honest answer +# is failure. A gate with nothing left to check reports green exactly as +# loudly as one that checked everything -- morph#466's shape, one level up. +expect_caught "the option() declarations become unparseable" \ + "edit CMakeLists.txt -e 's/^option(MORPH_BUILD_/OPTION_DISABLED(MORPH_BUILD_/' \ + && edit examples/vetted_hmac/CMakeLists.txt -e 's/^option(MORPH_BUILD_/OPTION_DISABLED(MORPH_BUILD_/'" \ + "found no option(MORPH_BUILD_...) declarations at all" + +# -- False positives --------------------------------------------------------- +# A default-ON option needs no workflow to name it: it is built by every job +# that does not turn it off. +expect_accepted "a newly declared option that defaults ON" \ + "printf '%s\n' 'option(MORPH_BUILD_TELEMETRY \"Build the telemetry sink\" ON)' >> CMakeLists.txt" + +# Coverage moving between jobs is not drift. The option is still built; the +# gate must not care which leg does it. +expect_accepted "an option enabled by a different job than before" \ + "edit .github/workflows/ci.yml -e '/^ *-DMORPH_BUILD_LOAD_TESTS=ON /d' \ + && printf '%s\n' ' -DMORPH_BUILD_LOAD_TESTS=ON \\' >> .github/workflows/ci.yml" + +if [ "$failures" -ne 0 ]; then + printf '\n%d case(s) failed.\n' "$failures" >&2 + exit 1 +fi + +printf '\nall cases passed.\n' From 92128cff4cf81eb396ec8d3d2a146faff8b8eba2 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Sun, 20 Sep 2026 10:33:43 +0200 Subject: [PATCH 2/2] ci: a red leg reports every test surface it could measure (fixes #618) Three jobs run disjoint test activities in sequence with no condition on the later steps, so the implicit `success()` means a failure in the first stops the second from running at all. The surface the second covers is then measured nowhere, and nothing in the log distinguishes "fine" from "never attempted" -- discovering it costs a full 26-74 minute cycle on the next push, including every green leg that had nothing to do with it. `!cancelled()` on the later step in each: - ladder-tests: "Run the scenario corpus against the built servers", a different surface from the ctest step above it. - ladder-sanitizers: "Test (the Qt transport suites, which no sanitizer leg ran)". This is the case that settles the argument -- by that step's own comment it is the only leg in CI that runs the Qt transport suites under any sanitizer. - linux-all-features: "Verify the fuzz replay covered the committed reproducers", which checks whether a guard went vacuous and cannot be inferred from the ctest step's result. `!cancelled()`, not `always()`. `always()` also fires during teardown of a cancelled run, which makes a leg that stopped within seconds of its siblings read as a defect rather than a cancellation -- the artefact this repository's own triage guidance warns about. `!cancelled()` runs on success or failure and stops on cancellation. Each condition keeps the guard that was already there (`steps.filter.outputs.run`, `matrix.fuzzers`), so nothing newly runs on a leg that was being skipped for its own reasons. Neither condition changes a job's conclusion: a red step keeps the job red. This makes a failing leg report more, not gate less. linux-coverage is deliberately not changed, and now says so in a comment next to the steps in question. Its "Generate coverage report" and the two steps after it consume the Test step's `.profraw` output rather than measuring a surface of their own; forcing them to run over truncated profile data would produce a coverage number computed from a partial pass, and a number nobody should trust is worse than no number. Written down because it is the part someone would otherwise "fix" later. Verification status: INFERRED, NOT REPRODUCED. The acceptance the issue asks for -- deliberately fail the first test step of one of these jobs and observe the later one still run and still report, on a red job -- requires a CI cycle against a deliberately broken branch, which this lane cannot run. What was measured: the edited ci.yml parses, and each of the three steps carries exactly `!cancelled() && ` while linux-coverage carries none. The behavioural claim rests on GitHub Actions' documented status-check-function semantics, the same standard as the issue's and the triage's own premise check. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk --- .github/workflows/ci.yml | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f608e297..2b17e776 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -658,6 +658,17 @@ jobs: run: | LLVM_PROFILE_FILE="build/clang-coverage/%p.profraw" ctest --preset clang-coverage + # Deliberately left on the implicit `success()`, unlike the three test + # steps morph#618 moved to `!cancelled()` (ladder-tests' scenario corpus, + # ladder-sanitizers' Qt transport suites, linux-all-features' fuzz-replay + # verification). This is a decision, not an oversight, and it is written + # down so it is not "fixed" later: this step and the two below it consume + # the Test step's `.profraw` output rather than measuring a surface of + # their own. Forcing them to run after a failed or truncated test pass + # would produce a coverage *number* computed from partial profile data -- + # and a number nobody should trust is worse than no number. The rule the + # three others follow is "a red leg reports everything it was able to + # measure"; here, nothing was. - name: Generate coverage report run: bash scripts/coverage.sh @@ -1325,8 +1336,18 @@ jobs: # A missing binary is not tolerated. run_scenarios.py exits 2 when a # server is absent and that is the right answer; softening it into a skip # would rebuild the blindness this step exists to end. + # `!cancelled()` rather than the implicit `success()`: this step tests a + # different surface from the ctest step above it (the scenario corpus + # against real servers, versus the ladder unit tests), so a red ctest + # skipping it leaves that surface unmeasured with nothing in the log + # distinguishing "fine" from "never attempted" -- and the next push pays + # a full 26-74 minute cycle to discover it. Not `always()`, which also + # fires during teardown of a *cancelled* run and manufactures a leg that + # stopped within seconds of its siblings looking like a defect rather + # than a cancellation. The job's conclusion is unchanged either way: a + # red step keeps the job red (morph#618). - name: Run the scenario corpus against the built servers - if: steps.filter.outputs.run == 'true' + if: "!cancelled() && steps.filter.outputs.run == 'true'" env: QT_QPA_PLATFORM: offscreen run: | @@ -1599,8 +1620,13 @@ jobs: # that goes stale silently: a renamed TEST_CASE would otherwise leave # ctest reporting success having run nothing (morph#466 is the same trap # one level down). + # `!cancelled()`, for the reason morph#618 gives and which bites hardest + # here: this is the only leg in CI that runs the Qt transport suites + # under any sanitizer, so if the ladder step above goes red and this one + # is skipped, that surface is measured nowhere at all. Not `always()` -- + # see ladder-tests' own note. The job still fails on a red step. - name: Test (the Qt transport suites, which no sanitizer leg ran) - if: steps.filter.outputs.run == 'true' + if: "!cancelled() && steps.filter.outputs.run == 'true'" env: QT_QPA_PLATFORM: offscreen # detect_leaks=0 for the reason the ladder step above gives: Qt's @@ -1871,8 +1897,14 @@ jobs: # A guard that never fires is indistinguishable from one that works, so # assert the fuzz replay actually ran the committed crash reproducers # rather than silently matching nothing (the `.txt`-vs-`.bin` glob bug). + # `!cancelled()`, per morph#618: this checks something the ctest step + # above cannot report on -- whether the replay matched the committed + # reproducers at all, the `.txt`-vs-`.bin` glob bug -- so skipping it + # when some unrelated test fails hides a guard going vacuous behind an + # unrelated red. Not `always()`; see ladder-tests' note. Still red on + # failure. - name: Verify the fuzz replay covered the committed reproducers - if: matrix.fuzzers == 'ON' + if: "!cancelled() && matrix.fuzzers == 'ON'" run: | ctest --preset ${{ matrix.preset }} -R fuzz -V > fuzz-replay.log 2>&1 status=0