bank/gui: bound parseMinor's input so an out-of-range amount is rejected rather than undefined (fixes #663)

[Yaraslaut]

Batch of two, of which one lands and one is handed back as mis-framed.

• bank GUI: parseMinor casts an unbounded double to int64_t; 1e30 in any amount field is UB #663 — fixed. Undefined behaviour on every bank GUI amount.
• HeaderFilterRegex excludes every examples/** header from clang-tidy: 49 findings from five of 737 TUs #664 — not fixed here. The mechanism it describes is real; the consequence it claims is not, and I measured that rather than inferred it. Details and numbers below, so the ticket can be re-framed rather than re-discovered.

Everything below was measured on this box: clang 22.1.8 (CI pins CLANG_VERSION: "22"), Qt 6.11.2, the clang-tidy job's own configure flags, from an empty build directory.

---

#663 — parseMinor converted an unbounded double to std::int64_t

examples/bank/gui/controllers/Format.hpp guarded only !ok || major < 0.0 and then cast major * scale + 0.5 to std::int64_t. QString::toDouble accepts 1e30, and five of the six call sites are GUI controllers reading a QML field with no validator.

Proof the test fails before the fix

Same test file, compiled against the header as it stands on 563502ab (extracted with git show) and against the fixed one. No sanitizer:

$ ./t_prefix
examples/bank/tests/gui/test_bank_gui_format.cpp:62: FAILED:
  CHECK_FALSE( parseMinor(... "1e30" ...).has_value() )
with expansion:
  !true
... (inf, nan, 9.3e16, 1e300, 1e17, 920000000000000000)
===============================================================================
test cases:  4 |  2 passed | 2 failed
assertions: 17 | 10 passed | 7 failed
exit=42

With -fsanitize=undefined -fno-sanitize-recover=undefined, the same binary does not get as far as reporting an assertion:

$ ./t_prefix_ubsan "parseMinor rejects amounts that do not fit in int64 minor units"
.../prefix/controllers/Format.hpp:76:38: runtime error: 1e+32 is outside the
range of representable values of type 'long'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior .../Format.hpp:76:38
exit=1

That is why every case asserts on the returned optional rather than on the arithmetic: the defect shows up as a wrong value on one configuration and as an abort on another, and only the optional assertion fails on both.

Against the fixed header, under UBSan: All tests passed (19 assertions in 4 test cases). Built and run for real, not only standalone — cmake --build build/clang-debug --target bank_gui_tests then the binary: All tests passed (21 assertions in 5 test cases).

Without a sanitizer, the pre-fix function returned -9223372036854775808 for 1e30, 1e300, inf, nan and 9.3e16 — the last is not an absurd magnitude, it is simply anything above ~9.2e16 major units once scaled by 100. inf and nan are both accepted by QString::toDouble, and nan walks through a < 0.0 guard because every comparison against a NaN is false; neither is mentioned in the issue.

Why the bound is not max() / scale

The issue and the triage both suggest std::numeric_limits<std::int64_t>::max() / scale. That form is not exact, in the unsafe direction: max() is 2^63-1, which is not representable as a double and converts upwards to 2^63, and dividing that by scale rounds a second time. A value landing exactly on such a bound still produces an out-of-range conversion.

So the check is on the scaled value against 0x1p63 — 2^63 is exactly representable — and it is written if (!(minor < kMinorUnitsBound)) so that a NaN is rejected rather than admitted. double arithmetic cannot trap, so computing the product first costs nothing.

Verified rather than argued: a sweep of 24576 doubles straddling the accept/reject edge for decimals 0, 1 and 2, under -fsanitize=undefined -fno-sanitize-recover=undefined, accepts exactly the values whose scaled form is below 2^63 and raises no diagnostic — checked=24576 accepted=12288 rejected=12288 -- no UB.

One thing this change does that is not an improvement

The old line carried a clang-tidy finding, reportable today:

Format.hpp:76:38: error: casting (double + 0.5) to integer leads to incorrect
rounding; consider using lround (#include <cmath>) instead
[bugprone-incorrect-roundings,-warnings-as-errors]

Splitting the expression to introduce the bound stops that check matching. The rounding behaviour is unchanged and is pinned by a test, but the check no longer points at it. The + 0.5 question is labelled weak by the filer and was explicitly out of scope for a UB fix, so it is filed rather than folded — see the issue linked below.

---

#664 — the premise does not hold for this repository's CI

The issue says the root .clang-tidy's HeaderFilterRegex: "include/morph/.*" means "findings in every header under examples/** are discarded from every job", and that this is why #663 went unreported. The first half of the mechanism is exactly as described. The conclusion is not, and the difference is testable.

What I measured first

I reproduced the job's configure (740 database entries, 423 in-workspace TUs, 282 under examples/) and ran clang-tidy over all 423 with --header-filter='.*', one output file per TU, de-duplicating findings by (header, line, column, check). Whole tree, not a sample:

distinct findings under --header-filter='.*' : 5577
  of which in a main file (always shown)     : 3010
  of which in an included header             : 2567

admitted by today's  HeaderFilterRegex 'include/morph/.*'      : 488
admitted by '.*' + exclude '/(build|_deps|vcpkg_installed)/'   : 1145

NEWLY ADMITTED : 657 findings in 159 headers
  633 findings in  150 header(s)  examples/
   21 findings in    6 header(s)  tests/
    1 findings in    1 header(s)  src/
    2 in system headers

So the filer's "49 findings across 12 headers" lower bound is 633 across 150 headers over the whole tree — about 13×. Top contributors are cert-dcl59-cpp (226), readability-identifier-length (80), readability-identifier-naming (73), readability-convert-member-functions-to-static (34), readability-redundant-member-init (33), bugprone-unchecked-optional-access (28).

The issue's warning against .* is also confirmed, with a number: .* alone admits 1422 further findings in 62 vendored headers, 1174 of them in the Lightweight ORM. And an inclusive alternation of first-party roots would be wrong on arrival, not merely stale — a header filter is an unanchored search over an absolute path, and (include/morph|src|tests|examples)/ matches 608 headers under build/clang-debug/_deps (244 glaze, 202 stdexec, 153 Lightweight), because FetchContent lays dependencies out as _deps/<name>-src/src/ and _deps/<name>-src/include/.

And then the part that changes the verdict

I built the widened config, the derived-polarity design and a behavioural gate for it, ran it green, and then ran the decisive check: does the pre-#664 regex actually hide anything from the one job that runs clang-tidy?

ci.yml's clang-tidy-diff is the only job that analyses the tree. Its own filter step says so explicitly: "A changed header is never dropped -- a header is never a translation unit." A changed header therefore reaches clang-tidy-diff.py, which names it on the command line — and clang tooling analyses a header named on the command line as that translation unit's main file. Main-file diagnostics bypass HeaderFilterRegex entirely.

Measured, on the unmodified pre-#664 config, with a diff containing nothing but one line of an examples/** header:

$ grep '^HeaderFilterRegex' .clang-tidy
HeaderFilterRegex: "include/morph/.*"

$ clang-tidy-diff.py -path build/clang-debug -p1 -extra-arg=-std=c++23 \
      -extra-arg=-Wno-missing-include-dirs -quiet < hdr_only.diff
examples/bank/gui/controllers/Format.hpp:100:10: error: variable name 'ok' is
too short, expected at least 3 characters [readability-identifier-length,...]
exit=1

The finding is reported, under the regex the issue says discards it.

The mirror case, also on the unmodified config: a diff that changes only a .cpp, whose TU includes examples/bank/include/bank/models/transaction_model.hpp (5 findings when analysed as an include) reports nothing — because clang-tidy-diff.py's -line-filter drops every finding in a file not in the diff, whatever the header filter says. Widening the regex does not change that either; I ran it both ways.

So HeaderFilterRegex has no observable effect on the only job in this repository that runs clang-tidy over the tree. In the diff job:

• a changed first-party header is linted today, as a main file, regex irrelevant;
• an unchanged header's findings are dropped by the line filter, regex irrelevant.

Which also disproves "#664 is why #663 sat unreported"

Analysing the pre-fix Format.hpp as its own TU under the pre-#664 narrow regex — i.e. exactly what CI does for a changed header:

Format.hpp:69:10: error: variable name 'ok' is too short ... [readability-identifier-length]
Format.hpp:76:38: error: '*' has higher precedence than '+' ... [readability-math-missing-parentheses]
Format.hpp:76:38: error: casting (double + 0.5) to integer leads to incorrect
                  rounding; consider using lround ... [bugprone-incorrect-roundings]

Three findings, one of them on line 76 — the line #663 is about — all reportable under the existing regex. Widening it adds only the three findings in the included bank/core/types.hpp, which the line filter discards anyway. #663 went unreported because nobody put that line in a diff, not because a regex swallowed it. (No check reports the out-of-range conversion itself under either regex; clang-tidy would not have found #663 in any configuration.)

So I landed nothing for #664

Widening the regex would have been a no-op in CI, and the gate I wrote for it would have asserted a property no job depends on — which is the failure mode AGENTS.md names first. Handing it back with the measurement instead, so it can be re-framed around the gap that is real: a finding in an unchanged first-party header is reported by no job at all. Closing that needs a whole-file clang-tidy leg, not a regex; its bill is now costed at the numbers above. The correction is posted on #664 and the follow-up is filed.

Reverted cleanly — this branch touches neither .clang-tidy nor scripts/, and nothing in ci.yml (held by #673).

---

Review reasoning, inline

/code-review and /simplify were not used (they fork background agents this lane may not dispatch), so the review pass is written out here.

• Does the new guard change any accepted value? No. The boundary sweep above is the evidence: 24576 doubles across three scales, accept/reject boundary exactly at "scaled value < 2^63", no UB. 12.34 -> 1234, 0 -> 0, " 7.5 " -> 750, 1200 at decimals=0 -> 1200 all unchanged, and pinned.
• Do the callers need changing? No. All six already go through .value_or(0) or an explicit if (minor); the new rejections join the existing ones.
• Is 0x1p63 the right constant, not an approximation? Yes, and deliberately not max() or max()/scale — reasoning and the rounding argument are in the header comment, because that is where the next reader will be standing.
• Could the test pass vacuously? It fails 7 assertions against the pre-fix header, which is the mutation. The whole-file build was also run: the CMake wiring is exercised, not just a standalone compile.
• Anything my own diff makes CI report? Checked with CI's own tool rather than assumed. clang-tidy-diff.py over the branch diff initially reported two bugprone-unchecked-optional-access findings in the new test (the check does not see a Catch2 REQUIRE guard through the macro expansion, and .value() is flagged the same as *). Rewritten to compare the optional directly, which asserts both halves at once; re-run is clean (exit=0). clang-format --dry-run -Werror clean over both touched files.
• Non-vacuity of that clang-tidy-diff run: re-introducing the unparenthesised major * scale + 0.5 makes it report readability-math-missing-parentheses and exit 1, so the check is reaching the header rather than skipping it.

Local pre-push gates, all green: check_tidy_suppression_scope.sh, check_qobject_moc_pairing.py and its --self-test, check_bidi_controls.py, check_nolint_directives.sh, check_rung_filters.sh.

Not verified

• CI itself. Nothing here has run on a runner; every number above is local, clang 22.1.8 against CI's pinned 22.
• The sanitizer legs still do not build bank_gui, so the UBSan evidence here comes from a hand-built binary rather than from a leg. That is the filer's own second observation on bank GUI: parseMinor casts an unbounded double to int64_t; 1e30 in any amount field is UB #663 and is filed separately.
• Whether bugprone-incorrect-roundings fires on this line under CI's Catch2 3.4.0 — the finding is in a non-test TU, so the 3.16.0 macro-expansion skew should not apply, but I did not install 3.4.0 to confirm.

Fixes #663. Refs #664.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner verification

The fix is right, and it corrects a bound I suggested that was subtly wrong. My triage of #663 proposed std::numeric_limits<std::int64_t>::max() / scale as "the honest upper bound". It is not: max() is 2^63−1, which is not representable as a double and converts upwards to 2^63, and a division adds its own rounding on top. The branch uses 0x1p63 instead, with the reasoning in the header:

"2^63 is exactly representable, so this literal is the one form of the bound that is exact."

That is the kind of correction I want from a lane rather than compliance.

I verified the guard's edge behaviour myself, under -fsanitize=undefined -fno-sanitize-recover=undefined:

  nan      -> rejected
  inf      -> rejected
  1e30     -> rejected
  9.3e16   -> rejected
  12.34    -> ACCEPTED
  UBSan exit=0

The NaN case is the one worth pointing at: if (!(minor < kMinorUnitsBound)) is negated deliberately, because NaN compares false against everything and nan < 0.0 is false, so the earlier guard lets it through. Written the natural way round (minor >= bound) NaN would pass. The comment says exactly that, which is the difference between a correct line and a correct line somebody will later "simplify".

And the bug was more reachable than the ticket said. 9.3e16 is not an absurd magnitude for a currency field, and it already overflowed. inf and nan are typeable. All four produced -9223372036854775808 into .value_or(0).

The before/after proof is the right shape — the new test compiled against the header extracted from 563502ab fails (CHECK_FALSE(...has_value()) / !true), and the same binary under UBSan does not reach an assertion at all (runtime error: 1e+32 is outside the range…). Those are two different demonstrations and both are needed: the first shows the test detects the defect, the second shows the defect is UB rather than a wrong answer.

The 24576-double sweep across the accept/reject edge for three decimals values, with no UB, is what makes "admits exactly the inputs below 2^63" a measurement rather than an argument.

#664 — handed back, and the hand-back is correct

I have closed it invalid and recorded the correction there: the regex is a no-op in CI both ways, because a changed header is analysed as a main file and main-file diagnostics bypass HeaderFilterRegex. ci.yml:2527 states the deciding half of that in its own words, and I had already verified the same fact myself two sweeps ago while checking PR #649's filter — and then triaged #664 valid without connecting them.

Building the widened regex, getting it green, then running the check that should have come first and reverting all of it is the expensive-but-correct order. The branch touches no .clang-tidy, no scripts/, no ci.yml — confirmed.

The tree-wide numbers survive the retraction and belong to #677/#580: 657 findings in 159 headers newly admitted, 633 of them in 150 examples/ headers — 13× the lower bound #664 cited — none of which CI would report, because no job lints unchanged code.

Not verified by me: the 5577/423-TU sweep, the bank_gui_tests run, and anything on a runner.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[Yaraslaut]

Runner: the red legs are environmental, re-run requested

Both runs are complete, so I read every failing job rather than inferring. Failing step, per leg:

Not one of them is a compile, a test or a lint finding. They are package installs, a vcpkg checkout, and a dependency-cache configure — the same class as #672, which now has four distinct upstreams recorded in one day.

The two signatures seen here:

• Install sccache / Install GCC 15 / Setup vcpkg — gzip: stdin: not in gzip format (a download that returned something other than a tarball, piped to tar unchecked) and pathspec 'D:\a\morph\morph\vcpkg' did not match any file(s) known to git.

• Configure — inside the vendored Lightweight ORM's own CMakeLists in the dep cache:

  CMake Error at ~/.cache/morph-dep-cache/Lightweight_bbb972a78e1962b9/CMakeLists.txt:98 (CPMAddPackage):
  -- reflection-cpp not found, downloading...
    Unknown CMake command "CPMAddPackage".
  

  That is a cached third-party tree missing CPM, not anything either branch changed.

Why I am confident this is not the branches: master's own 14:27Z run was success, and PR #671 settled 54/54 green at 18:12 — passing the very Configure step that failed here at ~15:21. The failure window is bounded and shared, which is what an environment looks like and not what a defect looks like.

Re-run requested. If a leg comes back red with a compile or test error, that is a real finding and I will treat it as one.

🤖 Generated with Claude Code

https://claude.ai/code/session_01VptDWG2fKr2vBnLSJcgzgW

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!