util: INT64_MIN is UB in four Rational operations and aborts in a fifth

[Yaraslaut]

Part of the sweep tracked in #518. Finding F19.

Summary

Rational(std::int64_t whole, DecimalPlaces) (rational.hpp:391) is the only constructor that does not call canonicalise(), so it is the one entry point that admits numerator == INT64_MIN:

constexpr Rational(std::int64_t whole, DecimalPlaces wantedPrecision) noexcept
    : numerator{whole}, decimalPlaces{detail::clampDecimalPlaces(wantedPrecision.value)} {}

Four downstream sites then negate it directly, and a fifth hands it to std::gcd.

Verification status

Reproduced. Revision: master @ 4017228d. Built with clang 21 / libstdc++ 16 under UBSan:

rational.hpp:972:37: runtime error: negation of -9223372036854775808 cannot be
                     represented in type 'std::int64_t'          [abs()]
rational.hpp:489:35: runtime error: negation of -9223372036854775808 ...  [operator-]
rational.hpp:499:66: runtime error: negation of -9223372036854775808 ...  [reciprocal()]
rational.hpp:896:60: runtime error: negation of -9223372036854775808 ...  [mulWouldOverflow]
/usr/include/c++/16/numeric:131: __abs_r(_Tp): Assertion
                     '__val != __int_traits<_Res>::__min' failed.

Input:

morph::math::Rational bad{std::numeric_limits<std::int64_t>::min(), DecimalPlaces{2}};
auto a = morph::math::abs(bad);
auto n = -bad;
auto r = bad.reciprocal();
auto p = bad * Rational{2, DecimalPlaces{2}};

reciprocal also produces a wrong answer where it does not trap: it returns 1/INT64_MAX — off by one ulp of denominator, with no clamp logged.

compareForSaturation (rational.hpp:750) reaches operator- via *this <=> -rhs, so the +=/-= saturation path is a fifth entry point.

Not verified: I did not check whether any in-tree caller constructs a Rational from INT64_MIN; this is reachable through the public constructor, not necessarily reached today. The numerator member is also public (rational.hpp:377-383), so it can be set directly.

Why this is a defect and not a design choice

docs/spec/util/rational.md acknowledges "INT64_MIN negation hazards" but lists three of the five sites, and none is fixed. The std::gcd case is the worst: it is a library precondition violation, not merely UB-on-overflow, so with _GLIBCXX_ASSERTIONS off it silently returns garbage and the product is quietly wrong rather than saturated.

The repository already has the correct helper — detail::absU64 — and already used it to fix exactly this pattern in formatRationalDecimal (morph#496). The sibling sites were not swept.

Suggested fix

Two independent changes:

1. Make the whole-integer constructor canonicalise like every other one. A delegating constructor keeps it constexpr and closes every path at once, matching what setWire already does:

   constexpr Rational(std::int64_t whole, DecimalPlaces p) noexcept
       : Rational{Numerator{whole}, Denominator{1}, p} {}

   canonicalise() already clamps INT64_MIN → -INT64_MAX and logs via reportClamp.
2. Defence in depth for the public-member case: route all five negation sites through detail::absU64.

What would change the verdict

• Close it if Rational{INT64_MIN, dp} is declared a precondition violation — but then the constructor should reject or clamp rather than admit it silently.
• Regression test, UBSan-gated: construct from INT64_MIN and exercise abs, unary -, *, reciprocal, checkedMul. tests/test_quantity.cpp:664 pins only the rendering path; tests/test_rational_checked.cpp:362,386 pin only the canonicalise clamp. Note this can only fail CI once the UBSan leg is able to fail at all — see the sanitizer finding.

[Yaraslaut]

Triage: rescope → valid (the rescope label was applied with no comment justifying it)

This issue carried triage: rescope since 2026-09-16 with zero comments — no
verdict was ever written to it, which the triage process itself treats as a
failure mode ("verdict by assertion"). Correcting that now.

Verification status

Reproduced by direct code reading against current include/morph/util/rational.hpp
(not rebuilt/run, since the issue's own UBSan repro already demonstrated the
crash precisely):

• The whole-integer constructor (rational.hpp:391) is confirmed as the
  issue describes: numerator{whole} with no call to canonicalise(), the
  only constructor that skips it.
• canonicalise() (rational.hpp:911) already contains the exact clamp the
  issue describes: numerator == minValue → -maxValue, with a reportClamp()
  log — confirmed by reading it in full, including its own comment
  explaining why (-INT64_MIN is unrepresentable).
• All five negation/UB sites confirmed present exactly as cited:
  operator-() (:488, Numerator{-numerator}), reciprocal() (:495-501,
  two direct negations), abs() (:970-975, -value.numerator),
  mulWouldOverflow() (:895-905, ternary -numerator), and
  compareForSaturation() (:749, reaches operator-() via *this <=> -rhs
  — not an independent implementation, just another caller of site 1).
• operator-()'s own doc comment already admits the bug: "Negating a
  Rational built from INT64_MIN overflows" — documented, not fixed.

Why valid, not rescope

No architecture invariant is implicated. This isn't a value-type-vs-policy
question (invariant 3) — Rational's own arithmetic producing UB on its own
public constructor's input is a defect in the type itself, not a policy
ActionTraits::fromJson or any codec boundary should be absorbing. It isn't
a spec/code disagreement either (invariant 5): docs/spec/util/rational.md
already flags "INT64_MIN negation hazards" as a known gap, incompletely
(three of five sites), not as a deliberate design position code and spec
disagree about.

Part 1 of the suggested fix — delegate the whole-integer constructor to the
existing Rational{Numerator, Denominator, DecimalPlaces} constructor — is
correct as written and closes the constructor path with zero new logic,
reusing canonicalise()'s existing clamp.

One refinement to Part 2 ("route all five negation sites through
detail::absU64"): that phrasing is imprecise for three of the sites.
absU64 returns a uint64_t magnitude — exact and safe, which is why it
already works for the pure-magnitude computation inside mulWouldOverflow's
gcd call. But operator-(), reciprocal(), and abs() don't want a
magnitude — they need to produce a new Rational whose numerator is the
negation, and -INT64_MIN has no exact int64_t representation at all, so
there is nothing for absU64 alone to hand back into a signed field. Those
three sites need the same clamp-and-log canonicalise() already does
(numerator == INT64_MIN → -INT64_MAX, via reportClamp()), not a bare
absU64 substitution. Concretely: a small detail::negateClamped(int64_t) -> int64_t helper mirroring canonicalise()'s existing inline clamp, used at
those three sites; mulWouldOverflow keeps using absU64 for its magnitude
comparison, unchanged from the suggestion.

This doesn't change the priority, the diagnosis, or that it should be built
as the issue's Part 1 + a corrected Part 2 — it changes one mechanism detail.

What would change this back

Re-open the rescope question only if Rational{INT64_MIN, dp} should instead
be a rejected precondition violation (the issue's own alternative) — but then
the constructor should reject or clamp rather than silently admit it, which
is a different, larger API-contract decision than fixing this UB.

[Yaraslaut]

Closing — Rational is being replaced with another library, so fixing this
in place is no longer the right use of effort.

The diagnosis stands as correct and verified (see the two comments above):
the whole-integer constructor skips canonicalise(), admitting INT64_MIN,
which four downstream negation sites then invoke UB on. That finding remains
valid evidence against the current Rational type — it just won't be
fixed here, since the type itself is being swapped out rather than patched.

Re-open if the replacement is abandoned and morph::math::Rational stays.