clang-tidy-diff run locally is silently blind to Catch2 TEST_CASE cognitive-complexity findings the CI job reports

[Yaraslaut]

A local clang-tidy-diff run over the same diff, with the same clang-tidy
version and the same job flags, can exit 0 on a diff the CI job fails — and it
does so silently, reporting nothing at all rather than reporting less. The
divergence is readability-function-cognitive-complexity inside Catch2
TEST_CASE bodies, and the variable is the Catch2 the workstation has
installed, which nothing in the repository pins or checks.

This is how morph#656's branch shipped a NOLINT reason asserting, in the
tree, that a neighbouring TEST_CASE "scores under the threshold and stays
covered" while it scored 87 against a threshold of 25. The claim was not
careless: the local gate agreed with it.

Verification status

Reproduced, on aa84b126 (PR #665's head, base 7ab4c7a9), Arch Linux,
clang-tidy 22.1.8 — the same major version ci.yml pins
(CLANG_VERSION: "22"), so this is not version skew in clang-tidy. The
configure is the clang-tidy-diff job's own, flag for flag, and the
clang-tidy-diff.py invocation is the job's own (-path build/clang-debug -p1 -extra-arg=-std=c++23 -extra-arg=-Wno-missing-include-dirs -quiet),
over git diff -U0 origin/master.

What differs between the two runs below is only which Catch2 headers are on
the include path.

Workstation Catch2 (3.16.0, /usr/include/catch2)

$ python3 /usr/share/clang/clang-tidy-diff.py -path build/clang-debug \
    -clang-tidy-binary clang-tidy -p1 -j 4 \
    -extra-arg=-std=c++23 -extra-arg=-Wno-missing-include-dirs -quiet \
    < one.diff
Running clang-tidy in 1 threads...

EXIT=0

The same run with Catch2 3.5.3 headers ahead of it (-isystem)

$ python3 /usr/share/clang/clang-tidy-diff.py -path build/clang-debug \
    -clang-tidy-binary clang-tidy -p1 -j 4 \
    -extra-arg=-std=c++23 -extra-arg=-Wno-missing-include-dirs \
    -extra-arg=-isystem.../Catch2-3.5.3/src \
    -extra-arg=-isystem.../b353/generated-includes -quiet \
    < one.diff
.../examples/bank/tests/gui/test_bank_gui_qml_behaviour.cpp:115:1: error: function 'dummyFunction72' has cognitive complexity of 87 (threshold 25) [readability-function-cognitive-complexity,-warnings-as-errors]
EXIT=1

That second line is character-for-character the error in the
clang-tidy-report artifact of CI run 35581623269, modulo the checkout path.

It is not that the score differs

clang-tidy computes the same score under both Catch2 versions. With the
workstation's 3.16.0 the finding exists and is then dropped:

$ clang-tidy -p build/clang-debug --checks='-*,readability-function-cognitive-complexity' \
    --config='{InheritParentConfig: true, CheckOptions: {readability-function-cognitive-complexity.Threshold: "1"}}' \
    --extra-arg=-std=c++23 --extra-arg=-Wno-missing-include-dirs --quiet \
    examples/bank/tests/gui/test_bank_gui_qml_behaviour.cpp
    (nothing for either TEST_CASE)

$ ... the same command with --system-headers added
.../test_bank_gui_qml_behaviour.cpp:115:1: error: function 'dummyFunction72' has cognitive complexity of 87 (threshold 1) ...
.../test_bank_gui_qml_behaviour.cpp:219:1: error: function 'dummyFunction76' has cognitive complexity of 45 (threshold 1) ...

So the check runs, the numbers are identical, and the whole difference is
whether ClangTidyDiagnosticConsumer classifies the finding as user code.

Mechanism, as far as it was established

checkFilters() is called for the error and for each of its notes, and both
LastErrorRelatesToUserCode and LastErrorPassesLineFilter are accumulated
across them — but a location inside a system macro returns early and
contributes nothing. A cognitive-complexity finding on a TEST_CASE is
therefore visible only through the notes that are not system macros: the
lambdas the test writes inside REQUIRE(pumpUntil([&]{ ... })), and its real
control flow. Which of those a given Catch2 release produces, and where it
puts them, is what changed between 3.5.x and 3.16.0.

This part is inferred from the behaviour plus a reading of
ClangTidyDiagnosticConsumer, not instrumented. The two exit codes above
are measured; the explanation for them is not.

A second, independent consequence of the same accumulation is measured, and
is worth writing down because it is counter-intuitive: the gate reports a
finding only when a changed line carries one of its user-code notes. A
one-line diff on REQUIRE(toastText != nullptr); — an assertion line inside a
case scoring 45 — reports nothing, because that line's only notes are system
macros. A one-line diff on REQUIRE(pumpUntil([&app] { ... })); in the same
case reports the 45 and exits 1. "I edited a line in an over-threshold
TEST_CASE" is therefore not the same condition as "the gate will fail".

Which Catch2 CI has

The artifact's macro-expansion notes place the runner's headers in the 3.5.x
series: catch_test_macros.hpp:142 is #define TEST_CASE( ... ) INTERNAL_CATCH_TESTCASE( __VA_ARGS__ ) and catch_unique_name.hpp:12/13/15
are ..._LINE2/..._LINE/INTERNAL_CATCH_UNIQUE_NAME, all of which match
Catch2 3.5.3 exactly. One line does not: the artifact renders
catch_test_registry.hpp:121 without the , __VA_ARGS__ that 3.5.3 has
there. Not verified: the exact package version of ubuntu-24.04's catch2.
Note that tests/.clang-tidy's header states it is 3.4.0, which cannot be
right — 3.4.0 has no CATCH_CONFIG_EXPERIMENTAL_STATIC_ANALYSIS_SUPPORT and
so cannot produce the dummyFunctionNN names the artifact is full of.

Why this matters beyond one comment

docs/ and several in-tree comments treat "run the clang-tidy job's configure
and clang-tidy-diff.py locally" as the way to check a branch before pushing.
For this check, on this file class, that procedure returns green on a red
diff. The failure is silent in both directions a reviewer might look: no
finding, no warning, no note that a check could not be evaluated.

What would change the verdict

• Close it if the job stops depending on the runner's Catch2 — e.g. the
  clang-tidy job builds Catch2 from the version CMakeLists.txt already
  pins for FetchContent (v3.8.1) rather than apt-get install catch2, so
  local and CI analyse the same expansion. A pin plus a documented "install
  this exact Catch2 to reproduce" would also close it.
• Close it if a gate makes the divergence loud instead of silent — for
  instance, the job asserting the Catch2 version it actually compiled against,
  so a local run against a different one is visibly not the same measurement.
• Re-open it if a second check turns out to diverge the same way; the
  mechanism above is not specific to cognitive complexity, only to findings
  whose evidence lives entirely in macro expansions.

[Yaraslaut]

Triage: valid — premise verified, and it is worse than the title says

local Catch2:      3.16.0
ci.yml installs:   sudo apt-get install -y ... catch2      (three jobs, UNPINNED)

So the divergence is not a one-off version gap: CI takes whatever ubuntu-24.04 ships on the day, and it can move without any change to this repository or any signal that it has.

triage: valid, bug, area: ci.

What makes this more than a nuisance: the failure direction is silent. A local clang-tidy-diff is green on a diff CI fails — it does not warn, it does not skip loudly, it reports success. Every lane in this repository is instructed to run gates locally before pushing, and for this class of finding that instruction currently produces false confidence rather than evidence. It has already done so at least once: the false claim corrected in PR #665 ("the sibling case above scores under the threshold") was the honest output of a local run that could not see the 87.

The repository already knows about the divergence — nine examples/*/tests/.clang-tidy copies say "which is why the finding reaches CI here and not on a workstation with a current Catch2". What it does not have is any mechanism that stops a contributor trusting the local result. Documented and undefended is the gap.

For whoever takes it

Two directions, and the issue names both:

• Pin the job's Catch2. Makes local and CI agree by construction, and makes the version a decision rather than an accident. The cost is that a pin is another thing to keep current, and this repository has a documented habit of lists that rot.
• Make the divergence loud. Have the local path detect that its Catch2 differs from the job's and say so. Cheaper to maintain, does not fix the asymmetry, but converts silence into a warning — which is the actual defect.

Do not "fix" it by telling contributors to run clang-tidy differently. An instruction is not a control; that is the distinction AGENTS.md is built around.

Note this interacts with #667: the version those nine files claim is itself wrong, so whatever pins or detects the version should also be what those comments cite, rather than a tenth hand-written copy of a number.

What would change the verdict

Close as invalid if the divergence turns out to be something other than Catch2's version — e.g. an include-path ordering artefact of the reproduction. The filer's own evidence is that putting 3.5.3 headers ahead on -isystem restores CI's behaviour, which is consistent with the version explanation but does not exclude a path-ordering one.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

PR #673 closes this by the second closing condition -- the clang-tidy-diff job now asserts the Catch2 it analyses against (scripts/check_catch2_pin.sh . --strict, run right after the apt install), and a workstation run of the same script prints the divergence rather than staying silent. The first closing condition (the job not depending on the runner's Catch2 at all) is filed as #674, since it needs examples/common/CMakeLists.txt, which PR #665 holds.

One correction to this ticket's text, established while working it and set out in full on #667: the runner's Catch2 is 3.4.0, not 3.5.x. The line this ticket reads as 3.5.x -- catch_test_registry.hpp:121 rendered without , __VA_ARGS__ -- is v3.4.0's text exactly; v3.5.3 has the , __VA_ARGS__. And the premise that 3.4.0 lacks CATCH_CONFIG_EXPERIMENTAL_STATIC_ANALYSIS_SUPPORT is false: v3.4.0 carries it at catch_test_registry.hpp:77/91/124 and #cmakedefines it at catch_user_config.hpp.in:172. Launchpad's noble Release pocket publishes catch2 3.4.0-1build1 with no -updates. So the note in this ticket's "Which Catch2 CI has" section, and the mechanism paragraph's "changed between 3.5.x and 3.16.0", should both read 3.4.0.

None of that touches this ticket's measurement: the two exit codes stand, and the divergence they show is what #673 makes loud. Also worth recording against the mechanism paragraph -- a minimal TEST_CASE whose complexity comes from control flow the test itself writes is reported locally against 3.16.0 (measured here: dummyFunction0 has cognitive complexity of 16 (threshold 1), no --system-headers). So 3.16.0 does not drop the check unconditionally; it drops findings whose evidence lives entirely in macro expansions, exactly as this ticket's mechanism section argues.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW