lint: make wrapped NOLINT directives apply, gate the ones that cannot, and clear clang-tidy batch A (fixes #627, fixes #600)

.github/workflows/suppression-guard.yml

@@ -0,0 +1,61 @@
+name: Suppression guard
+
+# Gates for the "a suppression that suppresses nothing" class of bug.
+#
+# NOLINTNEXTLINE applies to the next *physical* line. When the reason is wrapped
+# onto a second comment line, the directive annotates that comment, the findings
+# it names are still reported, and clang-tidy says nothing about it -- the
+# directive parses and the file looks annotated. Four such directives existed on
+# master and six findings leaked past three of them (#627).
+#
+# That is the failure mode AGENTS.md names first -- a control that reports
+# success while measuring nothing -- one level below the gate. It also matters
+# for the clang-tidy campaign in #580, whose agreed remedy is partly "write an
+# individually reasoned NOLINT": that campaign cannot proceed honestly on a tree
+# where NOLINTs silently fail to apply, so this gate is its precondition.
+#
+# WHY THIS IS ITS OWN WORKFLOW, AND WHERE IT BELONGS
+#
+# This job is a fast, dependency-free text scan that compiles nothing, which is
+# exactly .github/workflows/drift-guard.yml's remit, and it should be a job in
+# that file. It is here instead only because drift-guard.yml was held by an open
+# PR (#614) when this landed, and ci.yml by another (#623). Folding it into
+# drift-guard.yml once that has landed is tracked as its own issue; nothing
+# about the gate's behaviour changes when it moves.
+#
+# The self-test runs before the gate, as every other lint in this repository
+# does: a gate that detects nothing reports the same green as a clean tree, and
+# for this gate that would be the very defect it exists to catch, one level up.
+
+on:
+  push:
+    branches: [main, master]
+  pull_request:
+
+# Supersede a run a newer commit on the same ref has made obsolete -- see the
+# note in ci.yml. Cheap here, but the queue it shares is account-wide.
+concurrency:
+  group: suppression-guard-${{ github.ref }}
+  cancel-in-progress: true
+
+permissions:
+  contents: read
+
+jobs:
+  nolint-lint:
+    name: NOLINT directives that cannot take effect
+    runs-on: ubuntu-24.04
+    steps:
+      - name: Checkout
+        uses: actions/checkout@v4
+
+      # Drives the checker against tests/lint/nolint_directives/ first: the two
+      # effective shapes must be accepted, each inert shape must be rejected on
+      # its own and name its own file, and a directory with no directives at all
+      # must be rejected rather than called clean. Without this the step below
+      # would report green whether or not it still detects anything.
+      - name: Self-test the NOLINT-directive checker
+        run: bash scripts/test_check_nolint_directives.sh
+
+      - name: Check every NOLINTNEXTLINE annotates a line of code
+        run: bash scripts/check_nolint_directives.sh

examples/bookmarks/gui_lib/bookmark_forms_controller.hpp

@@ -114,7 +114,7 @@ class BookmarkFormsController {
     /// @param onReply    Success callback.
     /// @param onError    Failure callback.
     template <typename OnReply, typename OnError>
-    void submitIfValid(std::string actionType, std::string bodyJson, OnReply onReply, OnError onError) {
+    void submitIfValid(const std::string& actionType, const std::string& bodyJson, OnReply onReply, OnError onError) {
         try {
             dispatch(actionType, bodyJson)
                 .then(

include/morph/attributes.hpp

@@ -24,7 +24,7 @@
 /// diagnostic — a build break under the project's `-Werror`. So every use in
 /// morph goes through this macro, which expands to nothing on a compiler that
 /// does not know the attribute.
-#if defined(__has_cpp_attribute)
+#ifdef __has_cpp_attribute
 #if __has_cpp_attribute(clang::lifetimebound)
 #define MORPH_LIFETIMEBOUND [[clang::lifetimebound]]
 #elif __has_cpp_attribute(msvc::lifetimebound)

include/morph/detail/quantity_equation.hpp

@@ -27,6 +27,17 @@
 // opens a header on its own -- clang-tidy analysing a changed header, an IDE,
 // include-what-you-use -- otherwise sees `unknown type name 'ASTNode'` on
 // every line and reports a cascade of findings about code that compiles fine.
+//
+// misc-header-include-cycle sees the back-edge and is, as a graph statement,
+// correct: quantity.hpp -> quantity_equation.hpp -> quantity.hpp is a cycle.
+// What it cannot see is that the cycle is closed by `#pragma once` on the
+// second visit, which is the mechanism the paragraph above relies on rather
+// than an accident it survives. The check's remedy -- break the edge -- is the
+// state this file was deliberately moved away from, and it would restore the
+// standalone-analysis failure the paragraph describes. Suppressed here rather
+// than at the definition because this one include is the only cycle in the
+// tree; if a second appears, it should be argued for on its own.
+// NOLINTNEXTLINE(misc-header-include-cycle)
 #include "../util/quantity.hpp"
 #include "../util/rational.hpp"
 

include/morph/forms/forms.hpp

@@ -484,15 +484,26 @@ template <typename A>
 /// @brief Invokes `visitor.operator()<I>(name, member)` for every reflected
 ///        member of @p action (glaze pure reflection).
 template <typename A, typename Visitor>
-// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward, cppcoreguidelines-pro-bounds-avoid-unchecked-container-access)
-// — member-tie iteration
+// Neither forwarding reference is forwarded, and neither may be. `action` is
+// bound by `glz::to_tie` into a tuple of references that outlives this line and
+// is read member-by-member below; moving from it would leave the tie pointing
+// at a moved-from object. `visitor` is invoked once per reflected member by the
+// fold expression, so forwarding it would move from it on the first member and
+// call a moved-from callable for every one after. Both are `&&` to preserve the
+// argument's cv-qualification through the tie — a `const A&` must tie to const
+// members — not to enable a move. The directive stays on one physical line
+// deliberately; see the note at detail/fixed_string.hpp:48.
+// NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward)
 constexpr void forEachNamedMember(A&& action, Visitor&& visitor) {
     using Plain = std::remove_cvref_t<A>;
     constexpr auto memberCount = glz::reflect<Plain>::size;
     auto memberTie = glz::to_tie(action);
     [&]<std::size_t... I>(std::index_sequence<I...>) {
-        // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) — index bounded by
-        // reflect::size
+        // `I` is a pack of `std::index_sequence<memberCount>`, i.e. every value in
+        // [0, glz::reflect<Plain>::size), and `keys` is an array of exactly that
+        // size — the index cannot be out of range by construction. The directive
+        // stays on one physical line; see the note at detail/fixed_string.hpp:48.
+        // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access)
         (visitor.template operator()<I>(glz::reflect<Plain>::keys[I], glz::get_member(action, get<I>(memberTie))),
          ...);
     }(std::make_index_sequence<memberCount>{});

include/morph/journal/action_log.hpp

@@ -74,7 +74,7 @@ struct LogEntry {
     /// entry — one written before this field existed, or appended directly by
     /// application code — whose payload shape is unknown and therefore
     /// unverifiable. See `UnstampedPayloadPolicy` for what replay does with one.
-    std::string schema{};
+    std::string schema;
 
     /// @brief JSON-encoded result (`ActionTraits<A>::resultToJson`), captured after
     ///        successful execution. Empty when `outcome == Outcome::Failed`.
@@ -88,7 +88,7 @@ struct LogEntry {
 
     /// @brief `std::exception::what()` from the exception that rejected the
     ///        action. Empty unless `outcome == Outcome::Failed`.
-    std::string error{};
+    std::string error;
 
     /// @brief Auth principal from `morph::session::current()`, if any. Empty if unset.
     std::string principal;
@@ -102,7 +102,7 @@ struct LogEntry {
     ///        `morph::offline::QueueItem::idempotencyKey`'s exact contract: opaque,
     ///        stored verbatim, stable across restarts for one logical outbox row.
     ///        See `journal::OutboxRelay` (`outbox.hpp`) for how it's used.
-    std::string idempotencyKey{};
+    std::string idempotencyKey;
 
     /// @brief Line-format version this entry was written at.
     ///
@@ -135,7 +135,7 @@ struct LogEntry {
     /// entry at the point it is created, independent of whatever `seq` any
     /// sink later assigns it, and reuse that same identity as every cascaded
     /// entry's `causalParentId`.
-    std::string causalParentId{};
+    std::string causalParentId;
 };
 
 }  // namespace morph::journal

include/morph/journal/file_action_log.hpp

@@ -257,8 +257,8 @@ class FileActionLog : public IActionLog {
     /// @return Matching entries, in on-disk (append) order.
     [[nodiscard]] std::vector<LogEntry> entries(std::string_view entityKey = {}) const override {
         std::scoped_lock const lock{_mtx};
-        std::ifstream in{_path};
-        if (!in && std::filesystem::exists(_path)) {
+        std::ifstream input{_path};
+        if (!input && std::filesystem::exists(_path)) {
             // Distinguish "no journal yet" (absent: legitimately empty, and the
             // constructor's dedup rebuild depends on that) from "journal present
             // but unreadable". Returning {} for the second silently empties the
@@ -268,16 +268,21 @@ class FileActionLog : public IActionLog {
         }
         std::vector<std::string> lines;
         std::string line;
-        while (std::getline(in, line)) {
+        while (std::getline(input, line)) {
             if (!line.empty()) {
                 lines.push_back(line);
             }
         }
         std::vector<LogEntry> out;
         for (std::size_t i = 0; i < lines.size(); ++i) {
+            // .at() outside the try, not lines[i] inside it: the loop condition
+            // already bounds i, so the bounds check cannot fire, but if it ever
+            // could its std::out_of_range would be swallowed by the catch below
+            // and mis-reported as a malformed journal line.
+            std::string const& rawLine = lines.at(i);
             LogEntry entry;
             try {
-                entry = fromJson(lines[i]);
+                entry = fromJson(rawLine);
             } catch (const std::exception& exc) {
                 // A crash between `append`'s `fwrite` and the next flush can leave
                 // a truncated final line. Tolerate exactly that — skip a malformed

include/morph/qt/forms/forms_controller_core.hpp

@@ -93,7 +93,7 @@ class FormsControllerCore {
     /// @param onReply    Success callback.
     /// @param onError    Failure callback.
     template <typename OnReply, typename OnError>
-    void submitIfValid(std::string actionType, std::string bodyJson, OnReply onReply, OnError onError) {
+    void submitIfValid(const std::string& actionType, const std::string& bodyJson, OnReply onReply, OnError onError) {
         _handler.executeJson(actionType, bodyJson)
             .then([onReply = std::move(onReply)](std::string resultJson) mutable { onReply(std::move(resultJson)); })
             .onError([onError = std::move(onError)](const std::exception_ptr& err) mutable { onError(err); });
@@ -114,7 +114,8 @@ class FormsControllerCore {
     /// @param onReply       Success callback.
     /// @param onError       Failure callback.
     template <typename OnReply, typename OnError>
-    void fetchOptions(std::string optionsAction, std::string bodyJson, OnReply onReply, OnError onError) {
+    void fetchOptions(const std::string& optionsAction, const std::string& bodyJson, OnReply onReply,
+                      OnError onError) {
         _handler.executeJson(optionsAction, bodyJson)
             .then([onReply = std::move(onReply)](std::string resultJson) mutable { onReply(std::move(resultJson)); })
             .onError([onError = std::move(onError)](const std::exception_ptr& err) mutable { onError(err); });

include/morph/render/i18n.hpp

@@ -51,9 +51,34 @@ using TranslationProvider =
 /// @param derivedKey    The mechanically-derived key for this slot.
 /// @param schemaLiteral The schema's authored fallback text for this slot.
 /// @return The resolved display text.
+// NOLINTBEGIN(bugprone-easily-swappable-parameters)
+// `derivedKey` and `schemaLiteral` are adjacent `std::string_view`s, and the
+// check is right that swapping them would be silent and wrong: the derived key
+// would be rendered to the user as display text, and the schema's authored
+// title would be looked up in the catalog, miss, and fall back to the key. No
+// type or arity error would catch it.
+//
+// They stay in this order because the order *is* the contract. The three
+// parameters are the resolution chain in precedence order -- explicit key, then
+// derived key, then the schema literal as the fallback -- which is how the
+// @brief above states it, how the body below tries them, and how
+// docs/spec/forms/forms.md specifies it. The QML renderer carries a mirror of
+// this function with the same parameters in the same order
+// (src/qt/forms/qml/DynamicForm.qml, `resolveText(explicitKey, derivedKey,
+// literal)`), and the two are meant to be read against each other. Reordering
+// to break the adjacency here would desynchronise that pair and leave the
+// signature the only place in the stack that does not read as the chain --
+// trading a mistake that no caller in the tree is positioned to make for one a
+// reader of both renderers would.
+//
+// Strong types would remove the hazard outright, but a `TranslationKey` wrapper
+// on this seam would have to be threaded through every caller in morph::forms,
+// which is a design change to the renderer boundary and not a lint fix. If that
+// is ever done, delete this suppression rather than widening it.
 [[nodiscard]] inline std::string resolveText(const TranslationProvider& provider, std::string_view bcp47Locale,
                                              const std::optional<std::string>& explicitKey,
                                              std::string_view derivedKey, std::string_view schemaLiteral) {
+    // NOLINTEND(bugprone-easily-swappable-parameters)
     if (provider) {
         if (explicitKey.has_value()) {
             if (auto hit = provider(*explicitKey, bcp47Locale)) {

include/morph/version.hpp

@@ -3,6 +3,30 @@
 #pragma once
 #include <string_view>
 
+// The five macros below are the `#if`-testable half of morph's version API, and
+// being macros is the whole of what they are for. A downstream compiling against
+// two morph releases writes
+//
+//     #if MORPH_VERSION >= MORPH_MAKE_VERSION(1, 2, 0)
+//
+// which the preprocessor has to answer before any C++ declaration exists. An
+// enumerator or a `constexpr int` is invisible in a `#if`, and a `constexpr`
+// function cannot be called from one, so `MORPH_MAKE_VERSION` has to be
+// function-like for the same reason. The three checks below do not propose a
+// different spelling of this header; they propose removing the capability it
+// exists to provide. That is the meaning of "unfixable by construction", and it
+// is why this is a suppression rather than a deferral -- there is no later
+// version of this file in which the finding goes away.
+//
+// The C++-visible constants the checks ask for do exist: `morph::version::kMajor`
+// and friends, below, defined *from* these macros so the two cannot drift. Code
+// that does not need a `#if` should use those, and this header offering both is
+// the resolution, not a duplication.
+//
+// docs/spec/VERSIONING.md is the contract; tests/test_version.cpp pins these
+// against the top-level `project(morph VERSION ...)`.
+// NOLINTBEGIN(cppcoreguidelines-macro-to-enum, modernize-macro-to-enum, cppcoreguidelines-macro-usage)
+
 /// @brief morph's major version component (semantic-versioning MAJOR:
 /// incremented for a breaking source change to the stable public surface —
 /// see docs/spec/VERSIONING.md).
@@ -31,6 +55,8 @@
 /// `CMakeLists.txt` — the two are cross-checked by `tests/test_version.cpp`.
 #define MORPH_VERSION MORPH_MAKE_VERSION(MORPH_VERSION_MAJOR, MORPH_VERSION_MINOR, MORPH_VERSION_PATCH)
 
+// NOLINTEND(cppcoreguidelines-macro-to-enum, modernize-macro-to-enum, cppcoreguidelines-macro-usage)
+
 namespace morph::version {
 
 /// @brief morph's major version component, as a compile-time constant. See `MORPH_VERSION_MAJOR`.