clang-tidy #166: sweep test tree clean + promote clang-tidy CI job to a hard gate

.clang-tidy

@@ -55,6 +55,8 @@ Checks: >
   -cppcoreguidelines-avoid-magic-numbers,
   -cppcoreguidelines-init-variables,
   -cppcoreguidelines-pro-bounds-avoid-unchecked-container-access,
+  -cppcoreguidelines-pro-type-member-init,
+  -hicpp-member-init,
   -hicpp-named-parameter,
   -llvm-header-guard,
   -llvm-prefer-static-over-anonymous-namespace,

.github/workflows/main.yml

@@ -240,6 +240,22 @@ jobs:
         # compile_commands-update.sh runs bazel internally (--config=clang); feed it the disk cache
         # via the try-import'd .bazelrc.user. LLVM re-fetches each run (deliberately not cached).
         run: echo "common --disk_cache=$HOME/.cache/bazel-disk" >.bazelrc.user
+      - name: Materialize the headers the compile DB references
+        # A translation unit only needs its GENERATED / virtual-include headers to
+        # parse - xff/license/notice.h and xff/regex/backend.h come from the local
+        # @xff_extras_api module and are served through bazel-out `_virtual_includes`
+        # symlink forests that only exist once the owning cc_library is built. The
+        # compile DB records those bazel-out include paths but does not build them,
+        # so on a fresh runner clang-tidy aborts with `'xff/.../foo.h' file not
+        # found` and the fallout reads as phantom findings rather than the build gap
+        # it is (the "some sources were not generated" symptom). A plain `bazel build
+        # //...` (NOT --config=clang) materializes every genrule output and
+        # _virtual_includes forest at the DEFAULT-config path the DB uses -
+        # compile_commands-update.sh runs refresh_all without --config=clang, so the
+        # DB references `<cpu>-fastbuild`, and --config=clang would place the forests
+        # under a different, mismatched output dir. Headers do not depend on which
+        # compiler compiles the .cc, so the default toolchain build is correct here.
+        run: bazel build //...
       - name: Generate compile_commands.json
         # A gitignored local artifact, so build it here. The script fetches the hermetic LLVM and
         # records ITS clang, which is what makes the commands parseable by the matching clang-tidy.
@@ -265,14 +281,11 @@ jobs:
             echo "| Lint scope | \`${CLANG_TIDY_SCOPE}\` |"
           } >>"${GITHUB_STEP_SUMMARY}"
       - uses: pre-commit/action@v3.0.1
-        # Report-only until the remaining (test-file) findings are swept: .clang-tidy is
-        # WarningsAsErrors '*' and the test tree is not clean yet. Scoped to this step so a failed
-        # compile DB above still fails the job. Drop continue-on-error to promote to a hard gate.
-        continue-on-error: true
+        # Hard gate: .clang-tidy is WarningsAsErrors '*' and the tree is clang-tidy-clean, so any new
+        # finding fails this job (and thus `done`). The compile-DB build above gates too.
         with:
-          # Only the clang-tidy hook (every other hook ran in the pre-commit job). --hook-stage
-          # manual while the hook is opt-in; harmless once it becomes an automatic gate. Scope comes
-          # from the step above: changed sources on a branch, the whole tree on main.
+          # Only the clang-tidy hook (every other hook ran in the pre-commit job). Scope comes from
+          # the step above: changed sources on a branch, the whole tree on main.
           extra_args: clang-tidy --hook-stage manual ${{ env.CLANG_TIDY_SCOPE }}
 
   # Single required status check: one job that gates merge on the whole matrix,

.pre-commit-config.yaml

@@ -141,19 +141,19 @@ repos:
         files: ^tools/(trigger_release\.sh|trigger_release_test\.sh)$
         pass_filenames: false
       - id: clang-tidy
-        name: clang-tidy (local-only, opt-in)
+        name: clang-tidy (opt-in locally; gated by the CI clang-tidy job)
         description: |
           Runs clang-tidy over the given C++ sources via tools/clang_tidy.sh, a report-only
-          pass (no --fix, so it never rewrites the tree). It is a LOCAL-ONLY aid: it needs a
-          compile_commands.json (a gitignored local artifact) and a clang-tidy new enough for
-          this C++23 codebase, and SKIPS cleanly when either is missing - so it is a no-op on a
-          CI runner (no compile DB) and never forces the hermetic toolchain download. CI's hard
-          gate stays the bazel -Werror matrix. Run it explicitly with
-          `pre-commit run clang-tidy --all-files` (or on specific files with `--files ...`).
-          `stages: [manual]` keeps it OFF the automatic commit gate for now: the generated
-          compile DB still mis-orders an include path so clang-tidy aborts on files that pull
-          `<version>` / abseil headers (tracked in TODO.md). Once that and the clang-tidy-22
-          finding sweep land, drop `stages` to promote it to an automatic gate.
+          pass (no --fix, so it never rewrites the tree). It needs a compile_commands.json (a
+          gitignored local artifact; build it with ./compile_commands-update.sh) and a hermetic
+          clang-tidy (>= clang-22 for this C++23 codebase); it SKIPS cleanly when either is
+          missing, so it never forces the toolchain download on a fresh checkout.
+          `stages: [manual]` keeps it off the local commit hook (run it explicitly with
+          `pre-commit run clang-tidy --all-files --hook-stage manual`), but the dedicated CI
+          `clang-tidy` job owns it as a HARD GATE: that job builds the compile DB and runs this
+          hook with `--hook-stage manual`, and `.clang-tidy` is `WarningsAsErrors: '*'`, so any
+          finding fails CI. It is not run by trunk (trunk pinned clang-tidy 16, which mis-parses
+          C++23 - do not re-add it there).
         language: script
         entry: tools/clang_tidy.sh
         types_or: [c++]

STYLE_CPP.md

@@ -11,13 +11,17 @@ an AI assistant) can follow them without reverse-engineering the tooling.
   best-effort basis.
 - **`clang-format`** with [`.clang-format`](.clang-format) formats all C++ code. Run
   it; do not hand-format against it. CI rejects any reformatting diff.
-- **`clang-tidy`** with [`.clang-tidy`](.clang-tidy) (`WarningsAsErrors: true`) runs **locally**
-  via `trunk` (a `trunk check` and the editor daemon) against a `compile_commands.json` you
-  generate with `bazel run @bazel_compile_commands_extractor//:refresh_all`. **CI does not run it** (no
-  compile DB there), so CI's hard gate is the compiler `-Werror` in the bazel matrix; still treat
-  a clang-tidy finding as a must-fix before pushing. The enabled set is broad: `abseil-*`,
-  `bugprone-*`, `cppcoreguidelines-*`, `google-*`, `misc-*`, `modernize-*`, `performance-*`,
-  `portability-*`, `readability-*`.
+- **`clang-tidy`** with [`.clang-tidy`](.clang-tidy) (`WarningsAsErrors: '*'`) runs via the
+  opt-in `clang-tidy` pre-commit hook (`pre-commit run clang-tidy --all-files --hook-stage manual`,
+  which shells out to [`tools/clang_tidy.sh`](tools/clang_tidy.sh)) against a `compile_commands.json`
+  you generate with [`./compile_commands-update.sh`](compile_commands-update.sh). It is report-only
+  (never `--fix`) and needs a hermetic clang-tidy (>= clang-22 for this C++23 code); it skips cleanly
+  when either is missing. It is **not** run by `trunk` (trunk pinned clang-tidy 16, which mis-parses
+  C++23 and auto-applied build-breaking fixes - do not re-add it there). In **CI** the dedicated
+  `clang-tidy` job owns it: it builds the compile DB (`compile_commands-update.sh`, hermetic clang)
+  and runs this hook, report-only (`continue-on-error`) until the finding sweep lands, then a hard
+  gate. The enabled set is broad: `abseil-*`, `bugprone-*`, `cppcoreguidelines-*`, `google-*`,
+  `misc-*`, `modernize-*`, `performance-*`, `portability-*`, `readability-*`.
 
 ### What `.clang-format` decides (do not fight it)
 

compile_commands-update.sh

@@ -71,6 +71,18 @@ fi
 # exec configuration keep their command, so nothing leaves the compile DB.
 declare -a BCCE_ARGS=("--bcce-compiler=${CLANG}" "--bcce-prefer-target-config")
 
+# Force libc++ in the compile DB so clang-tidy / clangd ALWAYS parse against the
+# hermetic clang's own libc++, on every platform. Without this the recorded
+# commands inherit the toolchain default, which is libc++ on macOS but the system
+# libstdc++ on Linux. tools/clang_tidy.sh then prepends the hermetic libc++
+# `include/c++/v1` (so `#include <version>` beats helly25_mbo's plain-text `version`
+# file, which its -isystem'd repo root would otherwise shadow) - injecting libc++
+# headers into a libstdc++ parse. That stdlib mix silently corrupts clang-tidy's
+# type/member analysis (std::string_view resolving to int, phantom
+# const-correctness / convert-to-static / member-init findings). Pinning libc++
+# here makes the parse self-consistent everywhere, matching the clean macOS run.
+BCCE_ARGS+=("--bcce-copt=-stdlib=libc++")
+
 # The hermetic clang carries its own libc++ but no system C headers: without the
 # SDK sysroot its <locale> support headers fail on `'time.h' file not found`.
 # The bazel `--config=clang` toolchain supplies this itself; the extracted

tools/clang_tidy.sh

@@ -116,7 +116,39 @@ for CXX_V1 in \
   fi
 done
 
+# Checks with no meaning in test code, disabled for `*_test.cc` only (mirrors
+# helly25/mbo). Stating the rule once here - rather than a NOLINT repeated on every
+# test - keeps it in one place and covers new tests without anyone remembering to
+# annotate. `--checks` with only `-name` entries APPENDS to .clang-tidy's `Checks`
+# (removing those), so every other check still applies to tests.
+#   * readability-function-cognitive-complexity: a gtest TestBody's score comes from
+#     EXPECT_*/ASSERT_* macros expanding to branches, not from refactorable logic.
+#   * misc-override-with-different-visibility: our fixtures are `struct`s (AGENTS.md),
+#     so their SetUp()/TearDown() overrides are public while ::testing::Test declares
+#     them protected - a visibility change forced by convention, on every fixture.
+# concurrency-mt-unsafe is deliberately NOT disabled here: a test that touches process
+# environment (getenv/setenv) still carries a real MT hazard, so those few sites keep a
+# targeted, commented NOLINT rather than a blanket exemption for the whole test tree.
+readonly TEST_DISABLED_CHECKS='-readability-function-cognitive-complexity,-misc-override-with-different-visibility'
+
+declare -a SOURCES=()
+declare -a TESTS=()
+for FILE in "${@}"; do
+  case "${FILE}" in
+    *_test.cc | *_test.cpp | *_test.cxx) TESTS+=("${FILE}") ;;
+    *) SOURCES+=("${FILE}") ;;
+  esac
+done
+
 # Report only: --header-filter restricts diagnostics to this repo's own headers
 # (not the toolchain's force-included / system headers), -p points at the compile
-# DB. WarningsAsErrors in .clang-tidy makes any finding a non-zero exit.
-exec "${CLANG_TIDY}" --header-filter='(^|/)xff/' "${EXTRA_ARGS[@]}" -p . "${@}"
+# DB. WarningsAsErrors in .clang-tidy makes any finding a non-zero exit. Both groups
+# must run and a finding in either has to fail, so no `exec` (which would run one).
+STATUS=0
+if [ "${#SOURCES[@]}" -gt 0 ]; then
+  "${CLANG_TIDY}" --header-filter='(^|/)xff/' "${EXTRA_ARGS[@]}" -p . "${SOURCES[@]}" || STATUS=1
+fi
+if [ "${#TESTS[@]}" -gt 0 ]; then
+  "${CLANG_TIDY}" --header-filter='(^|/)xff/' --checks="${TEST_DISABLED_CHECKS}" "${EXTRA_ARGS[@]}" -p . "${TESTS[@]}" || STATUS=1
+fi
+exit "${STATUS}"

xff/cli/globals_test.cc

@@ -39,6 +39,7 @@ using ::testing::SizeIs;
 
 struct GlobalsTest : ::testing::Test {};
 
+// NOLINTNEXTLINE(readability-function-cognitive-complexity): a flat per-field validation sweep.
 TEST_F(GlobalsTest, EveryGlobalIsWellFormed) {
   EXPECT_THAT(Globals(), Not(IsEmpty()));
   for (const GlobalFlag& flag : Globals()) {

xff/cli/help_model_test.cc

@@ -34,6 +34,7 @@ using ::testing::SizeIs;
 
 // A std::visit overload set, so the walker below reads as one case per node kind.
 template<typename... Ts>
+// NOLINTNEXTLINE(misc-multiple-inheritance): the std overloaded-visitor idiom for std::visit.
 struct Overloaded : Ts... {
   using Ts::operator()...;
 };

xff/cli/help_render_test.cc

@@ -58,6 +58,19 @@ std::string RenderTopicDoc(std::string_view name) {
   return backend.Take();
 }
 
+// Renders a `--help=list` / `--help=all` / `--help=expressions` index document (IndexReference +
+// the plain backend), the same path the CLI uses, or "" when NAME is not an index. Keeps the
+// optional behind an if-guard so callers work with the rendered string directly.
+std::string RenderIndex(std::string_view name) {
+  const std::optional<Document> doc = IndexReference(name);
+  if (!doc.has_value()) {
+    return "";
+  }
+  PlainTextBackend backend;
+  RenderDocument(*doc, backend);
+  return backend.Take();
+}
+
 // Renders any model Document to plain text (the CLI path), for the index / full-reference
 // drift guards below.
 std::string RenderDoc(const Document& doc) {
@@ -71,7 +84,6 @@ using ::testing::Eq;
 using ::testing::HasSubstr;
 using ::testing::IsEmpty;
 using ::testing::Lt;
-using ::testing::Ne;
 using ::testing::Not;
 using ::testing::SizeIs;
 
@@ -112,9 +124,7 @@ TEST_F(HelpTest, UnknownTopicResolvesToNothingInTheModel) {
 TEST_F(HelpTest, ListIndexRendersEveryPrimaryAndTheTopicMap) {
   // `--help=list` is the whole-vocabulary index (the usage page): every expression
   // primary grouped by kind, plus the `--help=TOPIC` map.
-  const std::optional<Document> list = IndexReference("list");
-  ASSERT_THAT(list, Ne(std::nullopt));
-  const std::string out = RenderDoc(*list);
+  const std::string out = RenderIndex("list");
   EXPECT_THAT(out, AllOf(HasSubstr("Tests"), HasSubstr("Actions"), HasSubstr("Operators")));
   for (const registry::Descriptor& descriptor : registry::All()) {
     EXPECT_THAT(out, HasSubstr(descriptor.name)) << descriptor.name;
@@ -126,9 +136,7 @@ TEST_F(HelpTest, FullReferenceHasDetailsAllIndexIsSummariesOnly) {
   // The full reference (BuildReference, = --help=full) carries the long per-entry
   // explanations; `--help=all` is the same set summaries-only -- strictly shorter.
   const std::string full = RenderDoc(BuildReference());
-  const std::optional<Document> all_doc = IndexReference("all");
-  ASSERT_THAT(all_doc, Ne(std::nullopt));
-  const std::string all = RenderDoc(*all_doc);
+  const std::string all = RenderIndex("all");
   EXPECT_THAT(full, AllOf(HasSubstr("--sort"), HasSubstr("-regex"), HasSubstr("A config style sets")));
   EXPECT_THAT(all, AllOf(HasSubstr("--sort"), HasSubstr("-regex")));
   EXPECT_THAT(all, Not(HasSubstr("A config style sets")));  // summaries only, no detail prose
@@ -236,6 +244,7 @@ TEST_F(HelpTest, EnvironmentTopicListsTheVariables) {
   EXPECT_THAT(RenderTopicDoc("env"), Eq(env));  // the alias renders identically
 }
 
+// NOLINTNEXTLINE(readability-function-cognitive-complexity): a flat per-topic drift sweep.
 TEST_F(HelpTest, EveryAdvertisedTopicResolvesInTheModel) {
   // Drift guard: every advertised topic resolves to a non-empty model document via
   // TopicReference or IndexReference, and each alias renders identically. styles /
@@ -267,9 +276,7 @@ TEST_F(HelpTest, EveryAdvertisedTopicResolvesInTheModel) {
 TEST_F(HelpTest, ExpressionsIndexListsEveryPrimaryWithoutGlobals) {
   // `--help=expressions` is the annotated Tests/Actions/Operators list -- every
   // expression primary with its summary, but not the whole-run global flags.
-  const std::optional<Document> expr_doc = IndexReference("expressions");
-  ASSERT_THAT(expr_doc, Ne(std::nullopt));
-  const std::string expr = RenderDoc(*expr_doc);
+  const std::string expr = RenderIndex("expressions");
   EXPECT_THAT(expr, AllOf(HasSubstr("Tests"), HasSubstr("Actions"), HasSubstr("Operators")));
   for (const registry::Descriptor& descriptor : registry::All()) {
     EXPECT_THAT(expr, HasSubstr(descriptor.name)) << descriptor.name;
@@ -322,9 +329,7 @@ TEST_F(HelpTest, GlobalFlagResolvesByAliasAndDashless) {
 TEST_F(HelpTest, ListIndexIncludesGlobalGroupsAndEveryFlag) {
   // `--help=list` (the usage-page index) groups the whole-run flags by header and lists
   // every one, including a not-built extra flag (which carries its rebuild note).
-  const std::optional<Document> list = IndexReference("list");
-  ASSERT_THAT(list, Ne(std::nullopt));
-  const std::string index = RenderDoc(*list);
+  const std::string index = RenderIndex("list");
   EXPECT_THAT(index, AllOf(HasSubstr("Config"), HasSubstr("Traversal")));
   for (const GlobalFlag& flag : Globals()) {
     EXPECT_THAT(index, HasSubstr(flag.name)) << flag.name;
@@ -355,10 +360,8 @@ TEST_F(HelpTest, DetailedHelpShowsInfluenceCrossReferences) {
 
 TEST_F(HelpTest, InfluenceBlocksAreTheDetailTierOnly) {
   // The full reference (detailed tier) carries the influence blocks; --help=all (summaries) omits them.
-  const std::optional<Document> all_doc = IndexReference("all");
-  ASSERT_THAT(all_doc, Ne(std::nullopt));
   EXPECT_THAT(RenderDoc(BuildReference()), HasSubstr("Affected by:"));
-  EXPECT_THAT(RenderDoc(*all_doc), Not(HasSubstr("Affected by:")));
+  EXPECT_THAT(RenderIndex("all"), Not(HasSubstr("Affected by:")));
 }
 
 TEST_F(HelpTest, EveryAffectsTokenResolvesToARealEntry) {

xff/cli/pager_test.cc

@@ -25,6 +25,13 @@
 namespace xff::cli {
 namespace {
 
+// This file's fixture deliberately mutates the process environment (setenv/unsetenv/getenv) to
+// exercise pager resolution, saving and restoring each variable around a single-threaded test - so
+// the env calls are safe here. It is a gtest struct fixture, so SetUp/TearDown override the base's
+// protected members from a public scope and the saved-state members carry the private-style `_`
+// suffix. All three are test-fixture idioms, suppressed file-wide rather than per line.
+// NOLINTBEGIN(concurrency-mt-unsafe,misc-override-with-different-visibility,readability-identifier-naming)
+
 using ::testing::Eq;
 using ::testing::HasSubstr;
 
@@ -139,5 +146,6 @@ TEST_F(PagerCommandTest, ManPagerIsIndependentOfTheTextPager) {
   EXPECT_THAT(ResolvePagerCommand(PagerKind::kMan), HasSubstr("mandoc"));
 }
 
+// NOLINTEND(concurrency-mt-unsafe,misc-override-with-different-visibility,readability-identifier-naming)
 }  // namespace
 }  // namespace xff::cli

xff/color/color_test.cc

@@ -37,11 +37,11 @@ TEST_F(ColorTest, ResolveWhenLastOccurrenceWins) {
 }
 
 TEST_F(ColorTest, EnabledCombinesModeTtyAndNoColor) {
-  EXPECT_TRUE(Enabled(When::kAlways, /*tty=*/false, /*no_color=*/true));  // explicit wins over NO_COLOR
-  EXPECT_FALSE(Enabled(When::kNever, /*tty=*/true, /*no_color=*/false));
-  EXPECT_TRUE(Enabled(When::kAuto, /*tty=*/true, /*no_color=*/false));
-  EXPECT_FALSE(Enabled(When::kAuto, /*tty=*/false, /*no_color=*/false));  // not a terminal
-  EXPECT_FALSE(Enabled(When::kAuto, /*tty=*/true, /*no_color=*/true));    // NO_COLOR set
+  EXPECT_TRUE(Enabled(When::kAlways, /*stdout_is_tty=*/false, /*no_color_env=*/true));  // explicit wins over NO_COLOR
+  EXPECT_FALSE(Enabled(When::kNever, /*stdout_is_tty=*/true, /*no_color_env=*/false));
+  EXPECT_TRUE(Enabled(When::kAuto, /*stdout_is_tty=*/true, /*no_color_env=*/false));
+  EXPECT_FALSE(Enabled(When::kAuto, /*stdout_is_tty=*/false, /*no_color_env=*/false));  // not a terminal
+  EXPECT_FALSE(Enabled(When::kAuto, /*stdout_is_tty=*/true, /*no_color_env=*/true));    // NO_COLOR set
 }
 
 TEST_F(ColorTest, CodeForTypeUsesLsLikeScheme) {