ci/testkit: three gates that did not cover what they appeared to (fixes #651, fixes #652, fixes #655)

.github/workflows/ci.yml

@@ -2262,8 +2262,49 @@ jobs:
       #
       # Compiler caching (sccache/fastcache-cc) is still not set up here. That
       # step compiles 67 objects; a real build of this configure would compile
-      # the database's 703, so the caching steps would cost more setup than
+      # the database's 737, so the caching steps would cost more setup than
       # they could save.
+      #
+      # MORPH_BUILD_BANK_GUI is the last of the two flags "every optional
+      # feature is ON here" was not actually true about (morph#651). It
+      # defaults OFF (CMakeLists.txt), gates `add_subdirectory(gui)` in
+      # examples/bank/CMakeLists.txt, and the bank GUI's *tests* are gated on
+      # it too -- bank_gui_tests only exists `if(TARGET bank_gui_lib)`, which
+      # is that option's target. Eleven tracked sources therefore had no
+      # compile command in this job's database and were dropped, unanalysed,
+      # by the filter below: nine under examples/bank/gui/ and two under
+      # examples/bank/tests/gui/.
+      #
+      # Measured locally on 4563aff3, clang 22.1.8, Qt 6, this job's own flag
+      # set, cold build directory each time:
+      #
+      #   without BANK_GUI: 703 entries, 695 distinct in-workspace sources,
+      #                     270 under examples/; 63 of 473 tracked C/C++
+      #                     sources absent from the database; configure 63.7s
+      #   with BANK_GUI:    737 entries, 727 distinct in-workspace sources,
+      #                     281 under examples/; 52 absent; configure 64.4s
+      #
+      # The delta is exactly the eleven files, and the configure cost is under
+      # a second -- an `add_subdirectory` of Qt Quick targets nothing here ever
+      # builds. Both of the filter step's floors (600 in-workspace, 200 under
+      # examples/) move further from their limit, not closer.
+      #
+      # This needs nothing new installed: linux-all-features already builds
+      # this option on the same runner image with the same
+      # `jurplel/install-qt-action@v4 modules: qtwebsockets` install and the
+      # same apt set (morph#604 is why it is there), so the GUI's
+      # Qml/Quick/QuickControls2 requirement is already known to be met.
+      #
+      # What it does *not* do is clear the eleven files' existing findings.
+      # Measured on the same configure, whole-file, with this step's own
+      # `-extra-arg` pair: 97 findings inside the eleven sources themselves
+      # (44 cppcoreguidelines-pro-bounds-avoid-unchecked-container-access, 23
+      # performance-unnecessary-value-param, 9 misc-const-correctness, 8
+      # readability-identifier-length, 13 others), across ten of them --
+      # BankController.cpp is clean. clang-tidy-diff only reports on *changed*
+      # lines, so none of that goes red until someone edits one of these files
+      # on a line that carries a finding. That is morph#656, filed rather than
+      # folded in here; morph#646 is the precedent for keeping the two apart.
       - name: Configure (generates compile_commands.json over every optional feature)
         run: |
           cmake --preset clang-debug \
@@ -2276,6 +2317,7 @@ jobs:
             -DMORPH_BUILD_FUZZERS=ON \
             -DMORPH_BUILD_LADDER=ON \
             -DMORPH_BUILD_BANK_EXAMPLE=ON \
+            -DMORPH_BUILD_BANK_GUI=ON \
             -DCMAKE_C_COMPILER=clang-${{ env.CLANG_VERSION }} \
             -DCMAKE_CXX_COMPILER=clang++-${{ env.CLANG_VERSION }}
 
@@ -2511,9 +2553,12 @@ jobs:
           # symlinked checkout, a path this script fails to normalise the same
           # way) trips it too instead of silently skipping everything.
           #
-          # Measured with this job's own configure flags and clang 22: 703
-          # entries naming 695 distinct in-workspace sources, 270 of them under
-          # examples/. CI measured 690/276 at morph#481's revision. Both floors
+          # Measured with this job's own configure flags and clang 22: 737
+          # entries naming 727 distinct in-workspace sources, 281 of them under
+          # examples/ (703/695/270 before morph#651 added
+          # -DMORPH_BUILD_BANK_GUI=ON to the Configure step above -- that flag
+          # moves both numbers away from their floor, never towards it).
+          # CI measured 690/276 at morph#481's revision. Both floors
           # sit under both pairs with room for ordinary churn, and the
           # regression they exist to catch takes examples/ to 16, not to 199.
           MIN_ENTRIES = 600

.github/workflows/spec-sync.yml

@@ -92,4 +92,21 @@ jobs:
           # of paths, which is what lets scripts/test_check_spec_sync.sh drive
           # it against the commits it must reject rather than asserting it
           # works.
-          printf '%s\n' "$changed" | bash scripts/check_spec_sync.sh
+          #
+          # --allow-empty only when this step has *observed* the emptiness
+          # (morph#655). An empty path list is what a broken producer hands
+          # that script, and answering "OK, nothing to check" to it is a green
+          # tick for a check that read nothing -- so the script now refuses
+          # empty input unless the caller states it meant it. This step is
+          # entitled to state it: `set -euo pipefail` above means the
+          # `git diff` either produced this list or aborted the step, so an
+          # empty `$changed` here is a real net-zero diff (a pull request whose
+          # commits cancel out) and not a failure upstream. Passing the flag
+          # unconditionally would hand that entitlement back, and it is not the
+          # ordinary path anyway: every other run goes through the `else`.
+          if [ -z "$changed" ]; then
+              echo "The diff against the base is empty -- git diff succeeded and named no files."
+              printf '%s\n' "$changed" | bash scripts/check_spec_sync.sh --allow-empty
+          else
+              printf '%s\n' "$changed" | bash scripts/check_spec_sync.sh
+          fi

examples/TESTING.md

@@ -626,7 +626,15 @@ root `CMakeLists.txt` — don't repeat that eight times):
   too. There is deliberately **no** `examples/.clang-tidy`: one file there
   would cover every rung's `src/`, `include/` and `gui_lib/` as well, where a
   hand-written `a < b < c` is a real defect — `check_rung_filters.sh` fails if
-  one appears.
+  one appears. It also fails if any `.cpp` in one of those directories is not
+  a Catch2 translation unit: clang-tidy resolves configuration from the TU's
+  path, so a library source living beside the tests gets a suppression argued
+  as Catch2 idiom without being Catch2 at all. `morph_ladder_testkit`'s own
+  two TUs are in `examples/common/testkit_src/` rather than beside their
+  headers for exactly that reason. The headers themselves stay in
+  `testkit/` and are listed as sources of `morph_ladder_testkit`, which is
+  the only thing that keeps AUTOMOC seeing `fault_proxy.hpp`'s `Q_OBJECT`
+  once the same-basename pairing is gone.
 - `examples/common/` declares exactly three consumable targets:
   `morph_ladder_testkit` (morph + Catch2 + Qt), `morph_ladder_gui` (STATIC,
   `Qt6::Core` only, **no Catch2**, **no `Qt6::WebSockets`** — presenter rule

examples/bank/CMakeLists.txt

@@ -219,7 +219,13 @@ if(MORPH_BUILD_TESTS)
                 # while the audit itself needs Qt6::Core and nothing more.
                 # This is the same translation unit, not a copy of it, so the
                 # audit still has exactly one implementation.
-                ${PROJECT_SOURCE_DIR}/examples/common/testkit/qml_surface.cpp
+                #
+                # testkit_src/, not testkit/: the TU sits one directory up
+                # from its own header so that testkit/.clang-tidy's Catch2
+                # suppression cannot reach it (morph#652). The
+                # ${PROJECT_SOURCE_DIR}/examples/common include directory
+                # below is what resolves its <testkit/qml_surface.hpp>.
+                ${PROJECT_SOURCE_DIR}/examples/common/testkit_src/qml_surface.cpp
             )
             target_include_directories(bank_gui_tests PRIVATE
                 ${CMAKE_CURRENT_SOURCE_DIR}/tests

examples/common/CMakeLists.txt

@@ -181,9 +181,47 @@ endif()
 # regex/QMetaObject walker with no reason to be inlined into every rung's test
 # TU, and it deliberately stays out of apply_warnings()' reach by living in
 # this target (see the note below on Lightweight).
+#
+# Those two TUs live in testkit_src/ rather than beside their headers in
+# testkit/, and that is the whole of what morph#652 fixed. clang-tidy resolves
+# its configuration from the path of the translation unit it is analysing, and
+# testkit/.clang-tidy subtracts bugprone-chained-comparison on the argument
+# that the finding is Catch2's REQUIRE expansion rather than a defect. That
+# argument is true of the twenty-one Catch2 sources in that directory and says
+# nothing about a library TU with no REQUIRE in it -- yet while these two sat
+# there, the suppression covered them on the same terms, and the file's own
+# prose claimed it could not ("a hand-written `a < b < c` in the code under
+# test is still reported"). One directory up the suppression cannot reach
+# them, so the claim is true by construction instead of by assertion. The
+# headers stay in testkit/ because every consumer includes them as
+# <testkit/...>; only the two TUs moved.
+#
+# Which is why the two headers are listed as sources below, and they are not
+# there to be compiled. AUTOMOC finds a Q_OBJECT header two ways: next to the
+# .cpp of the same basename, or listed among the target's own sources. Moving
+# the TUs out of testkit/ broke the first, fault_proxy.hpp declares a QObject,
+# and nothing generated moc_fault_proxy.cpp any more -- a failure that is
+# invisible at compile time and that the static library archives happily. It
+# surfaces only at the first link that needs the vtable:
+#
+#   libmorph_ladder_testkit.a(fault_proxy.cpp.o): in function
+#     `morph::ladder::testkit::FaultProxy::FaultProxy(QUrl, QObject*)':
+#     undefined reference to `vtable for morph::ladder::testkit::FaultProxy'
+#     undefined reference to `...::FaultProxy::staticMetaObject'
+#     undefined reference to `typeinfo for ...::FaultProxy'
+#
+# Header entries are not compiled; they only join the AUTOMOC scan. This is
+# the same remedy, for the same reason, that cmake/morph_add_rung.cmake
+# already applies to every rung -- its _lib_headers glob exists because a
+# rung splits include/<rung>/app/app.hpp from src/app/app.cpp, and
+# pastebin::app::App is where this repository last hit it. Both headers are
+# listed rather than only the one carrying a Q_OBJECT today, so the move is
+# moc-neutral by construction instead of by which header happens to need moc.
 add_library(morph_ladder_testkit STATIC
-    testkit/fault_proxy.cpp
-    testkit/qml_surface.cpp
+    testkit_src/fault_proxy.cpp
+    testkit_src/qml_surface.cpp
+    testkit/fault_proxy.hpp
+    testkit/qml_surface.hpp
 )
 add_library(morph::ladder_testkit ALIAS morph_ladder_testkit)
 target_include_directories(morph_ladder_testkit PUBLIC ${CMAKE_CURRENT_SOURCE_DIR})

examples/common/testkit/.clang-tidy

@@ -24,6 +24,41 @@
 # keeps every other check the repository-root .clang-tidy enables: exactly one
 # entry is subtracted, and only here.
 #
+# ── What "only here" is worth, and how it is kept true (morph#652) ───────────
+#
+# That sentence was false for two years' worth of commits, in the way this
+# repository finds hardest to notice: the directory held the testkit *library*
+# as well as the testkit's tests. `fault_proxy.cpp` and `qml_surface.cpp` --
+# the two translation units of `morph_ladder_testkit`, neither containing a
+# single REQUIRE -- resolved their configuration from this file, so the check
+# was off for them on the strength of an argument about Catch2 that did not
+# apply to them at all. "The code under test" was in the directory with the
+# test, which is exactly the case the paragraph above says cannot happen.
+#
+# They now live in `examples/common/testkit_src/`, one directory up and
+# outside this file's reach, so the claim holds by construction. It is not
+# left to hold by assertion either: scripts/check_rung_filters.sh fails if any
+# .cpp in a directory carrying this suppression is not a Catch2 translation
+# unit -- no TEST_CASE, no SCENARIO, no catch2/ include -- which is the shape
+# a second library source arriving here would have. That check is the reason
+# the claim above can be read as a fact rather than an intention.
+#
+# What is *not* narrowed, and cannot be: the reusable headers beside the tests
+# (backend_rig.hpp, pump.hpp, db_fixture.hpp, ...) are library code too, and a
+# header reached from a TU under this directory is analysed with that TU's
+# configuration, not its own (morph#632 measured the same effect for
+# tests/.clang-tidy: 333 findings inside include/morph/** hidden across 25
+# headers). So a chained comparison written by hand in one of those headers is
+# unreported whenever the reaching TU is one of this directory's tests -- and
+# since every other consumer is a rung's test binary, reaching them from an
+# examples/<rung>/tests/ that carries its own copy of this same one entry, in
+# practice it is unreported from everywhere. That is the residue, stated
+# rather than narrowed, for morph#632's reason: a suppression whose reach is
+# written down is auditable, and one whose reach is guessed is not. It is
+# bounded by what the check is -- `clang-diagnostic-parentheses`, which this
+# file does not subtract, reports the same `a < b < c` shape independently
+# and was observed doing so on the probe morph#652 was measured with.
+#
 # One copy per test directory rather than one shared file, because no directory
 # contains every example's tests and nothing else. scripts/check_rung_filters.sh
 # fails if a test directory on the application ladder has no such file, so a new

examples/common/testkit/qml_surface.cpp → examples/common/testkit_src/qml_surface.cpp

@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: Apache-2.0
 
-#include "qml_surface.hpp"
+#include "testkit/qml_surface.hpp"
 
 #include <QDir>
 #include <QDirIterator>

examples/kanban/tests/test_kanban_qml_surface.cpp

@@ -156,7 +156,7 @@ TEST_CASE("Every kanban bridge exposes exactly the surface gui/qml binds, and no
     //          on each banner's `visible` is the probe; the `visible ? ... :
     //          ""` ternary on each `text` is the use). The audit treats a
     //          property's NOTIFY signal as covered by reading the property --
-    //          testkit/qml_surface.cpp's signal sweep does model this -- so
+    //          testkit_src/qml_surface.cpp's signal sweep does model this -- so
     //          the signal needs no exemption here, and claiming one would
     //          misdescribe a guard that is already doing its job.
     //   * OFF: neither property is compiled in, so no property read can cover

examples/ledger/tests/test_ledger_qml_surface.cpp

@@ -79,7 +79,7 @@ TEST_CASE("Every ledger bridge exposes exactly the surface gui/qml binds, and no
     //   * `busy` now gates a `BusyIndicator` in `LedgerView.qml`,
     //     `BudgetView.qml` and `RulesView.qml`. Its `busyChanged` NOTIFY needs
     //     no exemption of its own: the audit treats a property's NOTIFY as
-    //     covered by reading the property (testkit/qml_surface.cpp's signal
+    //     covered by reading the property (testkit_src/qml_surface.cpp's signal
     //     sweep), and that is exactly what these three views now do.
     //   * `ledgerBridge.refresh` is a "Refresh" button; `undoTransaction` is a
     //     journal-id field plus an "Undo" button, both in `LedgerView.qml`.

scripts/check_rung_filters.sh

@@ -253,15 +253,33 @@ shopt -u nullglob
 # test file written afterwards failed the gate on 14 findings, every one of
 # them a REQUIRE.
 #
-# Two conditions, not one, and the second is the load-bearing one. The file
-# must subtract the check -- and it must carry `InheritParentConfig: true`,
-# because without that key clang-tidy *replaces* the parent configuration
-# instead of extending it: a directory holding `Checks:
-# '-bugprone-chained-comparison'` alone runs with no checks enabled whatsoever
-# and reports green while linting nothing. A suppression that silences more
-# than the false positive is worse than the finding it hides, so it is checked
-# here rather than left to review.
+# Three conditions, not one, and the second and third are the load-bearing
+# ones. The file must subtract the check -- and it must carry
+# `InheritParentConfig: true`, because without that key clang-tidy *replaces*
+# the parent configuration instead of extending it: a directory holding
+# `Checks: '-bugprone-chained-comparison'` alone runs with no checks enabled
+# whatsoever and reports green while linting nothing. A suppression that
+# silences more than the false positive is worse than the finding it hides, so
+# it is checked here rather than left to review.
+#
+# The third condition is morph#652: every .cpp the suppression reaches must
+# actually be a Catch2 translation unit. clang-tidy resolves configuration
+# from the TU's path, so a non-Catch2 source in one of these directories gets
+# the suppression on the strength of an argument -- "this finding is Catch2's
+# REQUIRE expansion" -- that is not true of it. That is not hypothetical:
+# examples/common/testkit/ held `fault_proxy.cpp` and `qml_surface.cpp`, the
+# two translation units of the morph_ladder_testkit *library*, neither with a
+# REQUIRE anywhere in it, while that directory's .clang-tidy told every reader
+# the suppression could not reach the code under test. They now live in
+# examples/common/testkit_src/, and this condition is what stops the next one
+# arriving unnoticed.
+#
+# "Catch2 translation unit" is read from the source rather than from any build
+# file, because no build file is available to the jobs that run this gate: a
+# TEST_CASE/SCENARIO macro, or an include of catch2/, or CATCH_CONFIG_* for a
+# Catch2 main such as testkit_main.cpp. A library TU has none of the three.
 readonly tidy_false_positive="bugprone-chained-comparison"
+readonly catch2_marker='TEST_CASE|SCENARIO|CATCH_CONFIG|catch2/'
 
 check_test_dir_tidy_config() {
     local dir="$1" why="$2"
@@ -287,6 +305,44 @@ check_test_dir_tidy_config() {
         return
     fi
     note "${dir}/.clang-tidy subtracts ${tidy_false_positive} and inherits every other check"
+
+    # morph#652. Recursive: the suppression reaches every subdirectory too
+    # (examples/bank/tests/gui/ is the live case), and clang-tidy walks up
+    # from the TU, so a nested source is governed exactly as a top-level one
+    # is.
+    local -a sources=()
+    local source
+    while IFS= read -r source; do
+        [ -n "$source" ] || continue
+        sources+=("$source")
+    done < <(find "${repo_root}/${dir}" -type f -name '*.cpp' | sort)
+
+    checks=$((checks + 1))
+    if [ "${#sources[@]}" -eq 0 ]; then
+        fail "${dir}/ carries the ${tidy_false_positive} suppression but holds no .cpp at all -- either the suppression is pointing at the wrong directory, or this scan has stopped seeing the tree. Both leave the check below examining nothing while reporting green."
+        return
+    fi
+
+    # One grep over the whole list rather than one per file: `-L` prints the
+    # files that did *not* match, which is exactly the set wanted here, and a
+    # per-file loop costs a process per source for no extra information.
+    local -a non_catch2=()
+    while IFS= read -r source; do
+        [ -n "$source" ] || continue
+        non_catch2+=("${source#"${repo_root}/"}")
+    done < <(grep -LE -- "$catch2_marker" "${sources[@]}")
+
+    if [ "${#non_catch2[@]}" -ne 0 ]; then
+        fail "${dir}/.clang-tidy subtracts ${tidy_false_positive} as *Catch2 idiom*, but ${#non_catch2[@]} of the ${#sources[@]} .cpp it governs contain no Catch2 at all:
+$(printf '    %s\n' "${non_catch2[@]}")
+    clang-tidy resolves configuration from the translation unit's path, so each
+    of these is analysed with the check off on the strength of an argument
+    about REQUIRE that does not apply to it -- morph#652. Move the source out
+    of this directory (examples/common/testkit_src/ is the precedent), or, if
+    it really is a test, give it the Catch2 include or macro that says so."
+        return
+    fi
+    note "all ${#sources[@]} .cpp under ${dir}/ are Catch2 translation units, so the suppression's own justification covers every file it reaches"
 }
 
 while IFS= read -r rung; do