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

[Yaraslaut]

Two tickets, one commit each, in this order deliberately: batch A's remedy is
partly "write an individually reasoned NOLINT", and that cannot proceed
honestly on a tree where NOLINTs silently fail to apply.

fixes #627
fixes #600

---

1. Four suppressions that suppressed nothing, and a gate so it cannot recur

NOLINTNEXTLINE annotates the next physical line. Four directives had their
reason wrapped onto a second comment line, so each annotated that comment. The
directive parses, the file looks annotated, clang-tidy says nothing, and the
findings are still reported.

The four, fixed

site	what leaked
include/morph/forms/forms.hpp:487	cppcoreguidelines-missing-std-forward ×2
include/morph/forms/forms.hpp:494	cppcoreguidelines-pro-bounds-avoid-unchecked-container-access ×2
tests/oom_injector.cpp:107	cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory
tests/test_bridge_lifetime.cpp:519	nothing — inert and unnecessary

Each moves its reason above the directive rather than adding a
clang-format off guard, so the fix survives reformatting. fixed_string.hpp:48
is untouched: it is the prose documenting this hazard, and its clang-format
guard is the in-tree worked example of the other remedy.

Two judgement calls, stated rather than buried:

• forms.hpp:487 dropped a check it never needed. Only missing-std-forward
  fires on that line. Four NOLINTNEXTLINE directives are wrapped onto two lines and suppress nothing; six findings leak past them #627 flagged that the finding "may be a real defect (a
  forwarding reference that is never forwarded) rather than a false positive the
  author correctly waved off", and said someone should decide before restoring
  the suppression. Decided: neither parameter may be forwarded. action is bound
  by glz::to_tie into a tuple of references read member-by-member afterwards, so
  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. The && is there to carry cv-qualification through the tie. That reasoning
  is now the comment.
• test_bridge_lifetime.cpp:519 was both broken and unnecessary, as Four NOLINTNEXTLINE directives are wrapped onto two lines and suppress nothing; six findings leak past them #627
  suspected but could not confirm. Measured: cppcoreguidelines-owning-memory does
  not fire on placement new. Kept, now effective, so a later edit to that line
  cannot reintroduce the finding silently. (The owning-memory finding that does
  exist in that file is at :448, an unrelated delete.)

Before / after, measured

0e3b8823, clang-tidy 22.1.8, .clang-tidy unmodified, CI clang-tidy job's own
configure. Before — master's files, the leaked findings present:

include/morph/forms/forms.hpp:489:39: error: forwarding reference parameter 'action' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward,-warnings-as-errors]
include/morph/forms/forms.hpp:489:57: error: forwarding reference parameter 'visitor' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward,-warnings-as-errors]
include/morph/forms/forms.hpp:496:41: error: possibly unsafe 'operator[]' [cppcoreguidelines-pro-bounds-avoid-unchecked-container-access]
include/morph/forms/forms.hpp:496:66: error: possibly unsafe 'operator[]' [cppcoreguidelines-pro-bounds-avoid-unchecked-container-access]
tests/oom_injector.cpp:110:21: error: do not manage memory manually; consider a container or a smart pointer [cppcoreguidelines-no-malloc,-warnings-as-errors]
tests/oom_injector.cpp:110:9: error: initializing non-owner 'void *' with a newly created 'gsl::owner<>' [cppcoreguidelines-owning-memory,-warnings-as-errors]

After — all six gone; grep -E "missing-std-forward|no-malloc" over the same
logs returns nothing, and forms.hpp:49x is clean.

The gate, and its non-vacuity

scripts/check_nolint_directives.sh fails when a NOLINTNEXTLINE is followed by
a comment, a blank line, or nothing at all.

The strongest evidence it measures something is that it reproduces #627's own
finding from scratch. Run against the four files as they are on unmodified
origin/master, plus fixed_string.hpp:

$ bash scripts/check_nolint_directives.sh <master's include> <master's tests>

.../include/morph/forms/forms.hpp:487: NOLINTNEXTLINE is followed by a comment or blank line, so it suppresses nothing
    487 | // NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward, cppcoreguidelines-pro-bounds-avoid-unchecked-container-access)
    488 | // — member-tie iteration
.../include/morph/forms/forms.hpp:494: NOLINTNEXTLINE is followed by a comment or blank line, so it suppresses nothing
    494 |         // NOLINTNEXTLINE(cppcoreguidelines-pro-bounds-avoid-unchecked-container-access) — index bounded by
    495 |         // reflect::size
.../tests/oom_injector.cpp:107: NOLINTNEXTLINE is followed by a comment or blank line, so it suppresses nothing
    107 |     // NOLINTNEXTLINE(cppcoreguidelines-no-malloc, cppcoreguidelines-owning-memory) --
    108 |     // this *is* the process-wide operator new/delete pair; std::malloc/free
.../tests/test_bridge_lifetime.cpp:519: NOLINTNEXTLINE is followed by a comment or blank line, so it suppresses nothing
    519 |     // NOLINTNEXTLINE(cppcoreguidelines-owning-memory) -- placement new into
    520 |     // the mmap'd region above; destroyed via an explicit dtor call below, not
exit=1

Exactly four, at exactly the line numbers the issue names, and not
fixed_string.hpp:48 — which is in that same fixture set and is the prose
warning about this hazard, not a directive.

And the mutation test asked for: re-wrap one of the four on the fixed tree.

$ bash scripts/check_nolint_directives.sh

include/morph/forms/forms.hpp:496: NOLINTNEXTLINE is followed by a comment or blank line, so it suppresses nothing
    496 | // NOLINTNEXTLINE(cppcoreguidelines-missing-std-forward) -- the tie binds by
    497 | // reference and the visitor is called once per member
...
Scanned 165 NOLINTNEXTLINE directive(s) under: include src tests examples
exit=1

Reverted:

$ bash scripts/check_nolint_directives.sh
NOLINT directive lint OK: 165 NOLINTNEXTLINE directive(s), all annotating code.
exit=0

(166 after batch A adds one.)

scripts/test_check_nolint_directives.sh makes that permanent, in the shape
every other lint here uses — self-test first, then the gate. Fixtures in
tests/lint/nolint_directives/: both effective shapes accepted, each inert shape
(wrapped reason, blank line, last line of file) rejected on its own and naming
its own file, and a directory with no directives at all rejected rather than
called clean.

ok: valid fixtures accepted
ok: invalid fixture blank_line rejected
ok: invalid fixture last_line rejected
ok: invalid fixture wrapped_reason rejected
ok: directory with no directives rejected
all NOLINT-directive checker self-tests passed

Where the step lives, and why not where it belongs

It belongs in .github/workflows/drift-guard.yml. Both candidate homes are
held right now: #614 holds drift-guard.yml, #623 holds ci.yml, and both are
still open as of this push. So it is a new workflow,
.github/workflows/suppression-guard.yml, whose header says exactly that and
says folding it into drift-guard.yml changes nothing about its behaviour.
Tracked separately rather than left as a comment.

Stated residual

The scan anchors on NOLINTNEXTLINE as the first token after the comment
marker. clang-tidy itself is looser — it scans for the literal anywhere in a
comment — so // A NOLINTNEXTLINE directive must sit on ONE physical line... is,
to clang-tidy, a (vacuous, harmless) directive. Anchoring is deliberate: without
it, the in-tree documentation of this hazard could not be written. The residual
is a directive hidden mid-sentence; no such site exists in the tree. This is in
the script's header, not only here.

---

2. clang-tidy batch A: 20 → 0, and the precedents the other five batches inherit

Per #580's census and the owner's decision on it:
cppcoreguidelines-pro-bounds-avoid-unchecked-container-access stays enabled and
its findings get fixed — .at() or an individually reasoned NOLINT. A bare
NOLINT appears nowhere in this PR.

Batch A is 21 findings across 8 files; include/morph/render/locale_format.hpp
is excluded because PR #630 rewrites it and changes its answer, so its one
finding must be re-measured rather than inherited. That leaves 20 across 7
files.

Measurement

0e3b8823, clang-tidy 22.1.8, .clang-tidy unmodified, configured with the
clang-tidy job's own option set (ci.yml:2191). Nine TUs: the
verify_interface_header_sets stub for each batch-A header, plus
quantity.hpp's stub, examples/forms/gui_qml/FormsController.cpp and
tests/test_quantity.cpp for the findings a standalone header stub cannot reach
because it instantiates nothing. Deduplicated by path + line + column, as the
census did.

Before — the census row for row, with no adjustment:

TOTAL distinct findings: 20
    7  include/morph/version.hpp
    4  include/morph/journal/action_log.hpp
    4  include/morph/qt/forms/forms_controller_core.hpp
    2  include/morph/journal/file_action_log.hpp
    1  include/morph/attributes.hpp
    1  include/morph/render/i18n.hpp
    1  include/morph/detail/quantity_equation.hpp

After:

TOTAL distinct findings: 0

That zero is not a compile failure wearing a disguise. The same nine logs
still report 261 distinct findings elsewhere under include/morph/
(forms.hpp 104, quantity.hpp 46, views.hpp 40, …), no TU emitted a
clang-diagnostic-error, and every target below built and ran.

check → remedy → why

check	n	remedy	why
readability-use-concise-preprocessor-directives	1	fix	attributes.hpp's #if defined(__has_cpp_attribute) → #ifdef. Lexical; this is #600's finding.
readability-redundant-member-init	4	fix	action_log.hpp's std::string x{} members drop the initializer. std::string's default ctor is non-trivial, so aggregate and default initialization are unchanged either way.
readability-identifier-length	1	fix	file_action_log.hpp's std::ifstream in → input. Nothing about in was the vocabulary of an API, which is the only case where these renames lose.
performance-unnecessary-value-param	4	fix	forms_controller_core.hpp's submitIfValid/fetchOptions took std::string by value and passed it to executeJson, whose parameters are std::string_view. Never moved, so the copies bought nothing. const std::string&, not string_view: source-compatible, and it does not hand a view of a caller's temporary to an async dispatch path.
cppcoreguidelines-pro-bounds-avoid-unchecked-container-access	1	fix (.at())	file_action_log.hpp's lines[i]. Not a hot path — one journal read. Hoisted out of the surrounding try: the loop already bounds i so it cannot throw, but a .at() inside that try would have its std::out_of_range caught by catch (const std::exception&) and mis-reported as a malformed journal line.
cppcoreguidelines-macro-to-enum + modernize-macro-to-enum + cppcoreguidelines-macro-usage	7	reasoned suppression (NOLINTBEGIN/END)	Unfixable by construction. version.hpp's macros exist to be #if-testable: #if MORPH_VERSION >= MORPH_MAKE_VERSION(1, 2, 0) must be answerable by the preprocessor, where an enumerator is invisible and a constexpr function cannot be called. The checks do not propose a different spelling of the header; they propose removing the capability it exists to provide. The constants they ask for already exist beside them (morph::version::kMajor…), defined from the macros so the two cannot drift.
bugprone-easily-swappable-parameters	1	reasoned suppression	render/i18n.hpp's resolveText. The check is right that swapping derivedKey and schemaLiteral would be silent — the key would be rendered as display text and the literal looked up as a key. They stay because the order is the contract: the three parameters are the resolution chain in precedence order, as the @brief states it, as the body tries them, as docs/spec/forms/forms.md specifies it, and as DynamicForm.qml's resolveText(explicitKey, derivedKey, literal) mirrors it parameter for parameter. Reordering would desynchronise that pair. A TranslationKey strong type would remove the hazard outright, and is a design change to the renderer seam, not a lint fix — noted in the comment as the condition for deleting the suppression.
misc-header-include-cycle	1	reasoned suppression	quantity_equation.hpp's include back into quantity.hpp. The cycle is real as a graph statement and deliberate as a design — closed by #pragma once, and it is what makes the header analysable standalone (the file already carries a paragraph saying so). The check's remedy, break the edge, is the state this file was deliberately moved away from.

The precedent, stated once: prefer .at(); suppress only where the check is
wrong at that site, and say why in a paragraph that would survive someone
disagreeing with it. The bar is include/morph/render/locale_format.hpp:181.
Restating the check's name is not a reason.

One thing that moved outside include/morph/

examples/bookmarks/gui_lib/bookmark_forms_controller.hpp's submitIfValid
takes the same signature change. Its own doc comment says "Same body as
FormsControllerCore::submitIfValid", and that claim has to stay true. Not a
separate finding — the same declaration, duplicated.

What was built and run

cmake --build build/clang-debug --target all_verify_interface_header_sets \
      morph_forms_controller_core_tests ladder_bookmarks_gui_lib   ->  exit 0
cmake --build build/clang-debug --target morph_tests               ->  exit 0

./tests/morph_tests
  test cases:  1539 |  1538 passed | 1 failed as expected
  assertions: 22648 | 22647 passed | 1 failed as expected

./src/qt/forms/morph_forms_controller_core_tests
  All tests passed (9 assertions in 5 test cases)

Plus clang-format --dry-run --Werror clean on every touched file, and
check_spec_citations.sh, check_catch_test_names.sh, check_rung_filters.sh,
check_deprecated_markers.sh, check_ci_clang_pin.sh all green.

---

Review notes

Done inline rather than via /code-review.

• The .at() hoist is the only behaviour-adjacent change in the PR, and it is
  a strict improvement: it moves a call that cannot throw out of a catch that
  would have mishandled it if it could.
• const std::string& is an API change on a public template. It is
  source-compatible in both directions (submitIfValid(std::move(s), …) still
  compiles) and the spec cites these by parameter name, not type, so no spec
  text moves.
• Removing {} from four members is the one change where "mechanical" could
  hide a semantic difference. It does not: the members are std::string, whose
  default constructor is non-trivial, so aggregate init, value init and default
  init all produce an empty string with or without the initializer.
• The guard is the piece most likely to be wrong in an invisible way, which is
  why it gets a self-test, a vacuity case, a reproduction of the original four,
  and a mutation test, rather than "it passes".

What I did not verify

• Not run in CI. Every number here is from one Arch Linux machine,
  clang/clang-tidy 22.1.8, 12 cores. CI pins CLANG_VERSION: "22" and a
  different libstdc++; the integers may differ, the shape should not.
• Not a whole-tree clang-tidy sweep. Batch A's 20 → 0 is measured over the
  nine TUs that reach batch A's headers, not over all 695. A finding in one of
  these seven headers reachable only from some other include closure would not
  appear here. The before-run reproducing the census's per-file counts exactly is
  the evidence that the TU set is adequate, not a proof.
• readability-identifier-length on in was fixed by renaming without
  checking whether .clang-tidy's IgnoredVariableNames would have been the
  better answer for the batch-B sha1.hpp cases. That decision is deferred to
  batch B, where 26 of them live, and is not prejudged here.
• No --fix was run. Every edit is by hand.
• locale_format.hpp is not measured at all, deliberately; render/forms: escape the bidi controls in the QML i18n tests, and support native digit sets at both control edges (fixes #610, fixes #591) #630 owns it.
• The Qt/QML surfaces were not run, only compiled — morph_forms_controller_core_tests
  is the C++ half; the QML tests need a display.
• I did not check what the new workflow does to total CI wall time. It is one
  ubuntu-24.04 job running two bash scripts over ~700 files; measured locally at
  well under a second, but that is a local number.

Filed separately

Per AGENTS.md, findings from this work that are not this change:

• tests/.clang-tidy's check suppressions apply to headers reached from a
  tests/ TU, so a header gate driven by test TUs under-reports — measured, with
  a concrete case from this PR.
• Folding suppression-guard.yml into drift-guard.yml once ci: audit mutation_survivors.json's line citations instead of hoping (fixes #608) #614 lands.

Both linked below once opened.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk

[Yaraslaut]

Follow-up issues filed from this work, per AGENTS.md:

• tests/.clang-tidy's suppressions also silence include/morph headers, so a header gate driven by test TUs under-reports #632 — tests/.clang-tidy's suppressions also silence include/morph headers, so a header gate driven by test TUs under-reports. Reproduced; found while verifying Four NOLINTNEXTLINE directives are wrapped onto two lines and suppress nothing; six findings leak past them #627's forms.hpp:496 fix, which could not be reproduced from the obvious tests/ TU.
• Fold suppression-guard.yml's NOLINT job into drift-guard.yml once #614 lands #633 — fold suppression-guard.yml's job into drift-guard.yml once ci: audit mutation_survivors.json's line citations instead of hoping (fixes #608) #614 lands, so the placement reason does not survive only as a comment.

Also recorded on #580: include/morph/render/locale_format.hpp is excluded from batch A here, so batch A is 20 findings, not 21. Its row must be re-measured after #630 lands rather than reused.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AbwhcguQFkhvVi2AH19sWk

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!