From affb945dcd3a0f0f70f4e189c75385e68bf72847 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 06:54:44 +0200 Subject: [PATCH 1/3] ci: give the clang-tidy job the bank GUI its database was missing (fixes #651) `MORPH_BUILD_BANK_GUI` defaults OFF and the clang-tidy-diff job did not pass it, so eleven tracked sources -- nine under examples/bank/gui/ and two under examples/bank/tests/gui/, which exist only `if(TARGET bank_gui_lib)` -- had no entry in compile_commands.json. Since morph#649 they are dropped from the diff with a `::warning file=` rather than dying on a clang-diagnostic-error, so the gate has been silently reading eleven files less than it appears to. Measured on 4563aff3 with this job's own flag set, clang 22.1.8, Qt 6, a cold build directory each time: 703 entries / 695 distinct in-workspace sources / 270 under examples/ without the flag, against 737 / 727 / 281 with it. The delta is exactly those eleven files -- absent tracked sources go 63 to 52 of 473 -- and configure goes 63.7s to 64.4s, so the cost this option was once argued to carry is under a second here. Both of morph#649's floors (600 in-workspace, 200 under examples/) move away from their limit. Nothing new is installed: linux-all-features already builds this option on the same runner image, with the same install-qt-action invocation and the same apt set, which is what makes this a one-flag change rather than a new job. The eleven files are not clean. Whole-file, with this step's own -extra-arg pair: 97 findings inside the sources themselves, across ten of them. That is morph#656, filed rather than folded in -- clang-tidy-diff reports only on changed lines, so none of it goes red until someone edits one of these files, and clearing 97 findings in Qt GUI code needs a leg that compiles bank_gui, which this job deliberately is not. morph#646 is the precedent for keeping the flag and the debt apart. Slices (2) and (3) of morph#651 are deliberately left: the 36 sources under tests/lint/ and tests/compile_checks/ are not translation units by design and need recording, not fixing (morph#650), and the 13 WASM sources need a lint job that exists in no workflow -- a new job, not a flag. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- .github/workflows/ci.yml | 53 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 49 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2d1cd498..54112f4a 100644 --- a/.github/workflows/ci.yml +++ b/.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 From 512dc3fd4cf35e1863d3b16de1072ce1ace7c631 Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 07:05:52 +0200 Subject: [PATCH 2/3] testkit/ci: move the testkit library's two TUs out of the Catch2 suppression, and gate the class (fixes #652) examples/common/testkit/.clang-tidy subtracts bugprone-chained-comparison and argues for it as Catch2 idiom -- "an entry here is a check whose finding is test idiom, a property of Catch2, rather than a defect", "a hand-written `a < b < c` in the code under test is still reported". That directory also held morph_ladder_testkit's two translation units, fault_proxy.cpp and qml_surface.cpp, neither of which contains a REQUIRE. clang-tidy resolves configuration from the TU's path, so the suppression covered them on the strength of an argument that was not true of them, and the file told every reader it could not. Narrowed rather than reworded. The two TUs move to examples/common/testkit_src/, one directory up and outside this .clang-tidy's reach; their headers stay in testkit/ because every consumer includes them as , which is also what resolves qml_surface.cpp's own include after the move. The claim is now true by construction instead of by assertion. The other two options, and what they cost. Rewording alone is honest immediately and leaves a Catch2-idiom suppression sitting over non-Catch2 code, which is the state morph#632 objected to. Moving the *tests* into examples/common/testkit/tests/ is the same end state and the layout every other example already uses, but it is 21 renames that also silently break codecov.yml's two `examples/common/testkit/test_*.cpp` ignore globs -- a Codecov-side change nothing in this repository would report, which is morph#141's failure mode. This way is four files and no glob depends on it. Measured, on this tree, clang-tidy 22.1.8 (CI pins major 22), against the clang-tidy-diff job's own configure, with the repository's real .clang-tidy files rather than a --checks override. A `return aaa < bbb < ccc;` probe appended to the moved TU: examples/common/testkit_src/qml_surface.cpp:691:95: error: chained comparison 'v0 < v1 < v2' may generate unintended results, ... [bugprone-chained-comparison,-warnings-as-errors] and the control, the same probe in a TU still inside the suppressed directory, which reports the clang diagnostic and not the tidy check: examples/common/testkit/test_convergence.cpp:68:82: error: chained comparison 'X < Y < Z' does not behave the same as a mathematical expression [clang-diagnostic-parentheses] Both probes were removed afterwards; neither is on this branch. Before the move, `--checks=-*,bugprone-chained-comparison` over both library TUs reported nothing, so this buys no finding today -- what it buys is that the file's description of its own reach is checkable. And checked, because the next library source would arrive the same silent way. scripts/check_rung_filters.sh's check 5 gains a third condition: every .cpp a Catch2 suppression reaches, recursively, must itself be a Catch2 TU -- a TEST_CASE/SCENARIO, a catch2/ include, or CATCH_CONFIG_* for a Catch2 main. Across the nine suppressed directories that is 154 sources and, before this change, exactly two exceptions: the two this commit moves. A directory carrying the suppression and holding no .cpp at all fails too, since a scan that examines nothing reports "all Catch2" as loudly as one that examined everything. Mutation-tested: restoring qml_surface.cpp to testkit/ takes the gate from "All 58 rung-filter checks passed" to "1 of 58 ... failed", naming the file. scripts/test_check_rung_filters.sh pins all three new rejections and now copies the real test sources into its scratch tree rather than the .clang-tidy files alone -- a synthetic corpus would go on passing after the real tree had grown a library source in one of these directories, which is the failure this is about. In one `cp -r` per directory, not per file: per-file copying cost six thousand processes and took the self-test from 50s to five minutes. scripts/coverage.sh and examples/TESTING.md are updated where they describe the old layout; codecov.yml needs no change, since its globs name testkit/test_*.cpp and the moved files are under examples/common either way. The headers are listed as sources of morph_ladder_testkit, and that is what keeps the move from breaking the build. CMake's AUTOMOC documentation gives exactly two ways a Q_OBJECT header reaches the scan list: "all header files in the target's sources", and a same-base-name header found beside each C++ source. The move broke the second, fault_proxy.hpp declares a QObject, and nothing generated moc_fault_proxy.cpp any more. Compilation is untouched and the static library archives clean, so this is invisible until the first link that needs the vtable -- which is six CI legs, not one. Measured on a build directory configured from empty, clang 22.1.8 + Ninja, -DMORPH_BUILD_QT=ON -DMORPH_BUILD_LADDER=ON -DMORPH_LADDER_RUNGS=all -DMORPH_BUILD_TESTS=ON -DMORPH_BUILD_NET=ON -DMORPH_BUILD_OFFLINE_SQLITE=ON. Without the two header entries: examples/common/morph_ladder_testkit_autogen/mocs_compilation.cpp // No files found that require moc or the moc files are included $ nm -C examples/common/libmorph_ladder_testkit.a | grep staticMetaObject U morph::ladder::testkit::FaultProxy::staticMetaObject /usr/bin/ld: examples/common/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' /usr/bin/ld: ... undefined reference to `morph::ladder::testkit::FaultProxy::staticMetaObject' /usr/bin/ld: ... undefined reference to `typeinfo for morph::ladder::testkit::FaultProxy' clang++: error: linker command failed with exit code 1 With them, from a second empty directory and the same configure: examples/common/morph_ladder_testkit_autogen/C4OGJZPURZ/moc_fault_proxy.cpp 0000000000000038 D morph::ladder::testkit::FaultProxy::staticMetaObject 0000000000000070 D vtable for morph::ladder::testkit::FaultProxy 00000000000000e0 D typeinfo for morph::ladder::testkit::FaultProxy and the whole tree builds: 441 ninja edges, exit 0, every rung's test binary linked. moc writes `#include "testkit/fault_proxy.hpp"` -- the header is under the target's examples/common include directory, so CMAKE_AUTOMOC_PATH_PREFIX resolves it and nothing ascends; scripts/check_automoc_includes.sh over that tree reports 32 generated moc sources, none ascending. Header entries are not compiled; they only join the AUTOMOC scan. This is the remedy cmake/morph_add_rung.cmake already applies to every rung, for the same reason and with the same failure behind it -- its _lib_headers glob exists because a rung splits include//app/app.hpp from src/app/app.cpp, and pastebin::app::App is where this repository last hit this link error. Both headers are listed rather than only the one carrying Q_OBJECT today, so the split is moc-neutral by construction instead of by which header happens to need moc. The other two candidates. Moving the headers down beside their TUs rewrites every include in the tree and gives the .clang-tidy argument nothing it did not already have. An AUTOMOC search path does not exist: of the properties CMake offers, AUTOMOC_INCLUDE_DIRECTORIES sets the -I list handed to moc rather than the header-to-source pairing, CMAKE_AUTOMOC_RELAXED_MODE concerns "foo.moc" self-includes, and AUTOMOC_MACRO_NAMES, AUTOMOC_DEPEND_FILTERS and AUTOMOC_MOC_OPTIONS leave discovery alone -- so listing the headers is the documented mechanism, not a workaround for it. check 5 is unaffected and still holds: copying both TUs back into examples/common/testkit/ on this tree takes scripts/check_rung_filters.sh from "All 58 rung-filter checks passed" to "1 of 58 rung-filter check(s) failed", naming examples/common/testkit/fault_proxy.cpp and .../qml_surface.cpp. A header in that directory is not a translation unit and the scan does not see it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- examples/TESTING.md | 10 ++- examples/bank/CMakeLists.txt | 8 ++- examples/common/CMakeLists.txt | 42 ++++++++++- examples/common/testkit/.clang-tidy | 35 +++++++++ .../{testkit => testkit_src}/fault_proxy.cpp | 0 .../{testkit => testkit_src}/qml_surface.cpp | 2 +- .../kanban/tests/test_kanban_qml_surface.cpp | 2 +- .../ledger/tests/test_ledger_qml_surface.cpp | 2 +- scripts/check_rung_filters.sh | 72 ++++++++++++++++--- scripts/coverage.sh | 6 +- scripts/test_check_rung_filters.sh | 48 ++++++++++++- 11 files changed, 209 insertions(+), 18 deletions(-) rename examples/common/{testkit => testkit_src}/fault_proxy.cpp (100%) rename examples/common/{testkit => testkit_src}/qml_surface.cpp (99%) diff --git a/examples/TESTING.md b/examples/TESTING.md index 2c5c4da7..227768bc 100644 --- a/examples/TESTING.md +++ b/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 diff --git a/examples/bank/CMakeLists.txt b/examples/bank/CMakeLists.txt index a4f84b94..ae75f709 100644 --- a/examples/bank/CMakeLists.txt +++ b/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 . + ${PROJECT_SOURCE_DIR}/examples/common/testkit_src/qml_surface.cpp ) target_include_directories(bank_gui_tests PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/tests diff --git a/examples/common/CMakeLists.txt b/examples/common/CMakeLists.txt index 2458cca9..d996eabe 100644 --- a/examples/common/CMakeLists.txt +++ b/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 +# ; 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//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}) diff --git a/examples/common/testkit/.clang-tidy b/examples/common/testkit/.clang-tidy index 1bbb5028..75936551 100644 --- a/examples/common/testkit/.clang-tidy +++ b/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//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 diff --git a/examples/common/testkit/fault_proxy.cpp b/examples/common/testkit_src/fault_proxy.cpp similarity index 100% rename from examples/common/testkit/fault_proxy.cpp rename to examples/common/testkit_src/fault_proxy.cpp diff --git a/examples/common/testkit/qml_surface.cpp b/examples/common/testkit_src/qml_surface.cpp similarity index 99% rename from examples/common/testkit/qml_surface.cpp rename to examples/common/testkit_src/qml_surface.cpp index bb4d8b45..3d14b7cd 100644 --- a/examples/common/testkit/qml_surface.cpp +++ b/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 #include diff --git a/examples/kanban/tests/test_kanban_qml_surface.cpp b/examples/kanban/tests/test_kanban_qml_surface.cpp index ba7b6ced..d948e3cc 100644 --- a/examples/kanban/tests/test_kanban_qml_surface.cpp +++ b/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 diff --git a/examples/ledger/tests/test_ledger_qml_surface.cpp b/examples/ledger/tests/test_ledger_qml_surface.cpp index f1a7ce48..10a79950 100644 --- a/examples/ledger/tests/test_ledger_qml_surface.cpp +++ b/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`. diff --git a/scripts/check_rung_filters.sh b/scripts/check_rung_filters.sh index ef166039..3d8a4005 100644 --- a/scripts/check_rung_filters.sh +++ b/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 diff --git a/scripts/coverage.sh b/scripts/coverage.sh index a906fbc9..7d8e53b7 100644 --- a/scripts/coverage.sh +++ b/scripts/coverage.sh @@ -234,7 +234,7 @@ for _rung in "${RUNG_TEST_EXES[@]}"; do done done -# examples/common/testkit/ mixes real, reusable test-support headers/.cpp +# examples/common/testkit/ mixes real, reusable test-support headers # (backend_rig.hpp, db_fixture.hpp, strand_interleaver.hpp, ...) with actual # Catch2 test files (test_event_poller.cpp, test_presenter.cpp, ...) in the # same directory — unlike include/morph and examples/pastebin's SOURCES @@ -246,6 +246,10 @@ done # project has repeatedly had to hand-verify file by file. Test files # genuinely are not part of what examples/IMPLEMENTATION.md rule 5's 100% # bar means to hold to that standard — only the real testkit/GUI code is. +# The library's own two TUs are not in that directory at all any more: they +# live in examples/common/testkit_src/ so that testkit/.clang-tidy's Catch2 +# suppression cannot reach them (morph#652). They stay measured -- SOURCES +# names examples/common, and this regex matches only testkit/test_*.cpp. IGNORE_REGEX='.*/testkit/test_[^/]+\.cpp$' ${LLVM_PROFDATA} merge -sparse "${PROFILES[@]}" -o "$MERGED" diff --git a/scripts/test_check_rung_filters.sh b/scripts/test_check_rung_filters.sh index ecf7d3ea..208925ee 100644 --- a/scripts/test_check_rung_filters.sh +++ b/scripts/test_check_rung_filters.sh @@ -47,11 +47,28 @@ make_tree() { # the same "whatever the checker reads" rule as everything above -- the # directory has to exist for the check to fire at all, since a rung with no # tests/ yet is deliberately skipped. + # + # The .cpp files under each of those directories come too, because check 5 + # now reads them: every source the suppression reaches must be a Catch2 + # translation unit (morph#652). Copying the real ones rather than + # synthesising stand-ins keeps this self-test driving the gate against + # what the repository actually contains -- a synthetic corpus would go on + # passing after the real tree had grown a library source in one of these + # directories, which is the exact failure morph#652 records. + # Whole directory, in one `cp -r`, rather than the .clang-tidy alone: the + # sources beside it are read too now (check 5's third condition -- every + # .cpp the suppression reaches must be a Catch2 translation unit, + # morph#652). The real files rather than synthesised stand-ins, so this + # self-test keeps driving the gate against what the repository actually + # contains; a synthetic corpus would go on passing after the real tree had + # grown a library source in one of these directories, which is the exact + # failure morph#652 records. 2.3MB all told, and per-file copying here + # cost six thousand processes a run. for tidy in "${repo_root}"/examples/*/tests/.clang-tidy "${repo_root}"/examples/common/testkit/.clang-tidy; do [ -f "$tidy" ] || continue tidy_dir="$(dirname "${tidy#"${repo_root}/"}")" - mkdir -p "$dest/$tidy_dir" - cp "$tidy" "$dest/$tidy_dir/" + mkdir -p "$dest/$(dirname "$tidy_dir")" + cp -r "${repo_root}/${tidy_dir}" "$dest/${tidy_dir}" done } @@ -191,6 +208,33 @@ expect_caught "a tests/.clang-tidy that replaces the root config instead of exte "edit examples/lims/tests/.clang-tidy -e '/^InheritParentConfig:/d'" \ "has no 'InheritParentConfig: true'" +# morph#652's own defect, reintroduced: a library translation unit sharing a +# directory with the Catch2 tests, so the suppression covers it on the +# strength of an argument about REQUIRE that is not true of it. This is not a +# hypothetical shape -- examples/common/testkit/ held morph_ladder_testkit's +# two TUs for as long as that library existed, while the .clang-tidy beside +# them told every reader the suppression could not reach the code under test. +expect_caught "a non-Catch2 library source sharing the shared testkit's directory" \ + "printf '// SPDX-License-Identifier: Apache-2.0\\nnamespace morph { int probe() { return 0; } }\\n' > examples/common/testkit/probe_library_tu.cpp" \ + "examples/common/testkit/probe_library_tu.cpp" + +# The same shape one directory down, because the suppression reaches there +# too: clang-tidy walks up from the translation unit, so a nested source is +# governed exactly as a top-level one is. examples/bank/tests/gui/ is the live +# case this generalises. +expect_caught "a non-Catch2 library source in a subdirectory of a suppressed test directory" \ + "mkdir -p examples/bank/tests/gui && printf '// SPDX-License-Identifier: Apache-2.0\\nnamespace bank { int probe() { return 0; } }\\n' > examples/bank/tests/gui/probe_library_tu.cpp" \ + "examples/bank/tests/gui/probe_library_tu.cpp" + +# Vacuity guard on the scan that check 5's third condition runs. A directory +# carrying the suppression and holding no .cpp at all means either the +# suppression is pointing somewhere it does not belong, or this scan has +# stopped seeing the tree -- and a scan that examines nothing reports "all +# Catch2" exactly as loudly as one that examined everything. +expect_caught "a suppressed test directory the .cpp scan finds nothing in" \ + "find examples/common/testkit -name '*.cpp' -delete" \ + "holds no .cpp at all" + # The one-file shortcut that would satisfy every check above while taking the # check off every rung's src/, include/ and gui_lib/ as well. expect_caught "a single examples/.clang-tidy standing in for the per-directory ones" \ From 9cb41905d0162edf9b6d2da00873f3edca6bb5ed Mon Sep 17 00:00:00 2001 From: Yaraslau Tamashevich Date: Mon, 21 Sep 2026 07:07:32 +0200 Subject: [PATCH 3/3] ci: make check_spec_sync.sh refuse empty input unless asked for (fixes #655) The gate read a list of paths from stdin and answered `Spec sync OK: the change touches no files.` to an empty one. The sentence is honest -- it claims nothing was checked rather than claiming a check passed -- which is why this is narrower than the usual version of this defect. The exposure is not a lying gate but a broken upstream pipe producing a green tick. First, whether that exposure is live. CI's own invocation is not exposed, and this was measured rather than read off the YAML: spec-sync.yml's step runs under `set -euo pipefail`, assigns the diff to a variable, and pipes a `printf`, so a failing `git diff` aborts the step before the script is reached. Driving that exact shape with a bad ref: -- shape A: the workflow's own (assignment under set -e, then printf | checker) fatal: bad object deadbeefdeadbeef0000000000000000deadbeef step exit=128 The documented *manual* shape, in a shell with neither -e nor pipefail -- an interactive one, or a pre-push hook, which is how the filer found this -- is exposed exactly as filed: $ git diff --name-only HEAD | bash scripts/check_spec_sync.sh fatal: bad object deadbeefdeadbeef0000000000000000deadbeef Spec sync OK: the change touches no files. pipeline exit=0 and after this change: fatal: bad object deadbeefdeadbeef0000000000000000deadbeef ::error::spec-sync: the path list on stdin is empty, and --allow-empty was not given. pipeline exit=1 An explicit `--allow-empty` rather than making empty input an error outright: "this change touches no headers" is a real and common outcome the gate must still pass, so the point is to make silence something a caller states rather than something it falls into. An unrecognised argument exits 2 rather than being ignored, since a typo'd `--allow_empty` that the script shrugged off would wedge every legitimately empty invocation. spec-sync.yml passes the flag only when it has *observed* the emptiness -- `set -euo pipefail` means the git diff either produced the list or aborted the step, so an empty `$changed` there is a real net-zero diff (a pull request whose commits cancel out) and not an upstream failure. Passing it unconditionally would hand that entitlement straight back. Both paths are pinned in scripts/test_check_spec_sync.sh, plus whitespace-only input (which is what `printf '%s\n' "$changed"` produces for an empty variable), an unrecognised argument, and --allow-empty over a real violation, which must still be rejected. Rejecting empty is useless if --allow-empty stops working, and accepting it with the flag is useless if the flag is the only path anyone tests. Mutation-tested in both directions, since a self-test that passes against a feature that does nothing is the thing this repository distrusts. Forcing the empty branch to always allow: "NOT caught: an empty path list with no --allow-empty", 2 self-test checks fail. Forcing it to always reject: "FALSE POSITIVE: an empty path list with --allow-empty", 1 fails. Unmutated, all 23 pass. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW --- .github/workflows/spec-sync.yml | 19 ++++++++- scripts/check_spec_sync.sh | 53 +++++++++++++++++++++++- scripts/test_check_spec_sync.sh | 72 +++++++++++++++++++++++++++++++++ 3 files changed, 141 insertions(+), 3 deletions(-) diff --git a/.github/workflows/spec-sync.yml b/.github/workflows/spec-sync.yml index 2bcc612e..e5119b77 100644 --- a/.github/workflows/spec-sync.yml +++ b/.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 diff --git a/scripts/check_spec_sync.sh b/scripts/check_spec_sync.sh index 41a2a5a5..3d8673b8 100755 --- a/scripts/check_spec_sync.sh +++ b/scripts/check_spec_sync.sh @@ -1,5 +1,6 @@ #!/usr/bin/env bash # Usage: git diff --name-only HEAD | bash scripts/check_spec_sync.sh +# bash scripts/check_spec_sync.sh --allow-empty # silence must be asked for # # The header <-> spec sync gate .github/workflows/spec-sync.yml enforces: a # change to a header sub-domain must come with a change to the docs that @@ -34,8 +35,47 @@ # part that stops this recurring -- every directory under include/morph/ must # appear either in that table or in the exempt list below. A new sub-domain is # now a failure that names itself, instead of a silent exemption. +# +# ── Why empty input is an error unless asked for (morph#655) ──────────────── +# +# This gate reads a list of paths from stdin, so the question "what happens +# when the list is empty?" is the question "what happens when whatever +# produced it broke?". It used to answer `Spec sync OK: the change touches no +# files.` and exit 0. The sentence is honest -- it claims nothing was checked, +# rather than claiming a check passed -- but nobody reads the sentence, and a +# broken upstream then produces a green tick. +# +# Measured on 4563aff3, before the flag below existed. CI's own invocation was +# never exposed: .github/workflows/spec-sync.yml assigns the diff to a +# variable under `set -euo pipefail` and pipes a `printf`, so a failing +# `git diff` aborts the step with status 128 before this script is reached. +# The documented manual invocation, in a shell with neither -e nor pipefail -- +# an interactive one, or a pre-push hook -- is exposed exactly as filed: +# +# $ git diff --name-only HEAD | bash scripts/check_spec_sync.sh +# fatal: bad object +# Spec sync OK: the change touches no files. +# pipeline exit=0 +# +# So the fix is a flag rather than a hard error: "this change touches no +# headers" is a real and common outcome the gate must still pass, and the +# whole point is to make the empty case something a caller states rather than +# something it falls into. Without --allow-empty, empty input is a failure. set -euo pipefail +allow_empty=0 +for argument in "$@"; do + case "$argument" in + --allow-empty) allow_empty=1 ;; + *) + printf 'error: unrecognised argument: %s\n' "$argument" >&2 + printf 'usage: git diff --name-only HEAD | %s [--allow-empty]\n' \ + "${BASH_SOURCE[0]}" >&2 + exit 2 + ;; + esac +done + cd "$(git rev-parse --show-toplevel)" # ── The table: ||... ─────────────────── @@ -81,8 +121,17 @@ exempt_subdomains="detail qt" changed="$(cat)" if [ -z "${changed//[[:space:]]/}" ]; then - echo "Spec sync OK: the change touches no files." - exit 0 + if [ "${allow_empty}" -eq 1 ]; then + echo "Spec sync OK: the change touches no files (--allow-empty given)." + exit 0 + fi + echo "::error::spec-sync: the path list on stdin is empty, and --allow-empty was not given." + echo " This gate checks a list of changed paths. An empty list is indistinguishable" + echo " from a broken producer -- a failed \`git diff\`, a mistyped base ref, a" + echo " pipeline in a shell without \`set -o pipefail\` -- and answering OK to that" + echo " is a green tick for a check that read nothing (morph#655)." + echo " If the emptiness is genuine and expected, say so: pass --allow-empty." + exit 1 fi fail=0 diff --git a/scripts/test_check_spec_sync.sh b/scripts/test_check_spec_sync.sh index 84f384f2..18887c6f 100755 --- a/scripts/test_check_spec_sync.sh +++ b/scripts/test_check_spec_sync.sh @@ -59,6 +59,35 @@ expect_accepted() { fi } +# The same two, for the cases that turn on an *argument* rather than on the +# path list. Separate helpers rather than an extra parameter on the two above, +# so that the fifteen existing call sites keep saying exactly what they say. +expect_rejected_with_args() { + local description="$1" paths="$2" expected="$3"; shift 3 + local output + if output="$(printf '%s\n' "$paths" | bash "$checker" "$@" 2>&1)"; then + fail "NOT caught: ${description} -- the gate passed an invocation it must reject" + return + fi + if printf '%s' "$output" | grep -qF "$expected"; then + note "rejected: ${description}" + else + fail "rejected for the WRONG reason: ${description} -- no diagnostic containing '${expected}':" + printf '%s\n' "$output" >&2 + fi +} + +expect_accepted_with_args() { + local description="$1" paths="$2"; shift 2 + local output + if output="$(printf '%s\n' "$paths" | bash "$checker" "$@" 2>&1)"; then + note "accepted: ${description}" + else + fail "FALSE POSITIVE: ${description} -- the gate rejected an invocation it must accept:" + printf '%s\n' "$output" >&2 + fi +} + # The live gap morph#560 reported, in its minimal form. expect_rejected "a morph::net header change with no spec change" \ "include/morph/net/detail/ws_frame.hpp" \ @@ -110,6 +139,49 @@ expect_accepted "a qt and detail change with no spec change" \ expect_accepted "a change touching no headers at all" \ "$(printf 'README.md\ntests/test_bridge.cpp')" +# ── Empty input: both paths pinned, because only one of them is obvious ───── +# +# morph#655. An empty path list is what a broken producer hands this gate -- a +# failed `git diff`, a mistyped base ref, a pipeline in a shell without +# `set -o pipefail`. It used to answer `Spec sync OK: the change touches no +# files.` and exit 0, which is honest and still a green tick for a check that +# read nothing. It is now an error unless the caller asks for it. +# +# Both directions are pinned here deliberately. Rejecting empty input is +# useless if --allow-empty stops working, because every caller that +# legitimately has nothing to check would then be wedged; and accepting it +# with the flag is useless if the flag is the only path anyone tests, because +# a gate that quietly went back to accepting silence would pass that test too. +expect_rejected "an empty path list with no --allow-empty" \ + "" \ + "the path list on stdin is empty, and --allow-empty was not given" + +expect_accepted_with_args "an empty path list with --allow-empty" \ + "" --allow-empty + +# Whitespace-only is the same case: `printf '%s\n' "$changed"` with an empty +# `$changed` is exactly what .github/workflows/spec-sync.yml pipes, so the +# rejection has to survive the newline that shape adds. +expect_rejected "a path list of nothing but whitespace" \ + "$(printf ' \n\t\n ')" \ + "the path list on stdin is empty, and --allow-empty was not given" + +# An unrecognised argument must not be silently ignored: a typo'd +# `--allow_empty` that the gate shrugged off would fail every legitimately +# empty invocation, and a typo'd anything-else would hide whatever the caller +# meant to ask for. +expect_rejected_with_args "an unrecognised argument" \ + "README.md" \ + "unrecognised argument: --allow_empty" \ + --allow_empty + +# The flag must not become a way to pass a real violation. It governs the +# empty case and nothing else. +expect_rejected_with_args "--allow-empty over a real violation" \ + "include/morph/net/detail/ws_frame.hpp" \ + "include/morph/net/** changed but none of its spec paths did" \ + --allow-empty + # ── Cases that need a mutated tree ────────────────────────────────────────── # # The checker resolves its own root with `git rev-parse` and reads the tree for