Fix sanitizer, package consumer, and static analysis CI

- isolate allocator probes from ThreadSanitizer
- disable unit tests in the installed-package job
- export the C++20 requirement to downstream consumers
- resolve clang-tidy diagnostics without weakening checks
- document sanitizer coverage and package verification

Author: Onwcan

.github/workflows/ci.yml

@@ -121,7 +121,9 @@ jobs:
       # downstream.
       - name: Build and install
         run: |
-          cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_INSTALL_PREFIX="$PWD/staged"
+          cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \
+            -DMOTIONKIT_BUILD_TESTS=OFF \
+            -DCMAKE_INSTALL_PREFIX="$PWD/staged"
           cmake --build build -j "$(nproc)"
           cmake --install build
 
@@ -136,12 +138,26 @@ jobs:
           target_link_libraries(consumer PRIVATE motionkit::core)
           EOT
           cat > /tmp/consumer/main.cpp <<'EOT'
-          #include <cassert>
-          #include "motionkit/core/se3.hpp"
+          #include "motionkit/core/frame_graph.hpp"
           int main() {
             using namespace motionkit;
             const SE3 pose(SO3::rotZ(1.0), Vec3{1.0, 2.0, 3.0});
-            assert((pose.inverse() * pose).isApprox(SE3{}, 1e-12, 1e-12));
+            if (!(pose.inverse() * pose).isApprox(SE3{}, 1e-12, 1e-12)) {
+              return 1;
+            }
+            FrameGraph frames;
+            const auto base = frames.declareRoot("base");
+            if (!base) {
+              return 2;
+            }
+            const auto tcp = frames.declareFrame("tcp", base.value, pose);
+            if (!tcp) {
+              return 3;
+            }
+            const auto resolved = frames.lookup(base.value, tcp.value);
+            if (!resolved || !resolved.value.isApprox(pose, 1e-12, 1e-12)) {
+              return 4;
+            }
             return 0;
           }
           EOT

CMakeLists.txt

@@ -13,6 +13,8 @@ else()
 endif()
 
 option(MOTIONKIT_BUILD_TESTS      "Build unit tests"           ${MOTIONKIT_IS_TOP_LEVEL})
+option(MOTIONKIT_BUILD_ALLOCATION_TESTS
+       "Build global-allocation probe tests"                   ON)
 option(MOTIONKIT_BUILD_BENCHMARKS "Build micro-benchmarks"     OFF)
 option(MOTIONKIT_ENABLE_WERROR    "Treat warnings as errors"   ON)
 
@@ -49,6 +51,7 @@ target_include_directories(motionkit_core
   PUBLIC
     $<BUILD_INTERFACE:${PROJECT_SOURCE_DIR}/include>
     $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>)
+target_compile_features(motionkit_core PUBLIC cxx_std_20)
 target_link_libraries(motionkit_core PRIVATE motionkit::warnings)
 set_target_properties(motionkit_core PROPERTIES
   OUTPUT_NAME motionkit-core

CMakePresets.json

@@ -49,7 +49,8 @@
       "cacheVariables": {
         "CMAKE_BUILD_TYPE": "Debug",
         "CMAKE_CXX_FLAGS": "-fsanitize=thread -fno-omit-frame-pointer",
-        "CMAKE_EXE_LINKER_FLAGS": "-fsanitize=thread"
+        "CMAKE_EXE_LINKER_FLAGS": "-fsanitize=thread",
+        "MOTIONKIT_BUILD_ALLOCATION_TESTS": "OFF"
       }
     },
     {
@@ -116,4 +117,4 @@
       "configurePreset": "tsan"
     }
   ]
-}
+}

README.md

@@ -24,8 +24,11 @@ Eigen, no KDL, no Pinocchio — the algorithms are the point.
 | WP-06 | Hand-eye, TCP and base-frame calibration | Planned |
 | WP-12 | CUDA batch IK and collision checking | Planned |
 
-77 tests, all passing, under GCC and Clang in Debug and Release, plus ASan,
-UBSan and TSan.
+77 tests, all passing under GCC and Clang in Debug and Release. ASan and UBSan
+exercise the full suite. TSan exercises the 73 ordinary tests; the four
+allocator-interposition tests run in a dedicated executable and are excluded
+from TSan because both the tests and the sanitizer runtime replace the global
+allocation functions.
 
 ---
 
@@ -40,7 +43,9 @@ cmake --build --preset debug
 ctest --preset debug
 ```
 
-Other presets: `release`, `asan`, `tsan`, `tidy`.
+Other presets: `release`, `asan`, `tsan`, `tidy`. The `tsan` preset intentionally
+runs 73 tests: the four tests that instrument global allocation are a test-harness
+incompatibility with TSan, not an exemption for production code.
 
 Before pushing, run the formatter -- CI enforces it:
 
@@ -162,11 +167,11 @@ test wrong.
 |---|---|
 | GCC + Clang × Debug + Release | `-Wconversion` and `-Wold-style-cast` fire on different constructs per compiler |
 | `-Werror` with `-Wconversion -Wsign-conversion -Wold-style-cast -Wshadow` | Silent narrowing in a pose pipeline is a field failure, not a warning |
-| ASan + UBSan, `-fno-sanitize-recover=all` | A UBSan finding fails the build rather than printing a note |
-| TSan | Ahead of the threaded executor in WP-08 |
+| ASan + UBSan on all 77 tests, `-fno-sanitize-recover=all` | A UBSan finding fails the build rather than printing a note |
+| TSan on the 73 ordinary tests | Ahead of the threaded executor in WP-08; the four allocator-interposition tests are excluded because TSan defines the same global allocation hooks |
 | clang-tidy, `--warnings-as-errors=*` | Rule set and exclusions justified in ADR-0002 |
-| clang-format `--dry-run --Werror` | Formatting is not a review topic |
-| **install + downstream consumer compile** | Caught a real bug on first run: the exported target was `motionkit::motionkit_core` while in-tree consumers used the `motionkit::core` alias. Every `find_package` downstream would have failed, and no unit test could have seen it |
+| `scripts/format.sh --check` with clang-format 18 | Formatting is not a review topic, and CI runs the same check developers run |
+| **install with repository tests off + downstream consumer compile and run** | Exercises only the installed package contract; it caught a real bug on first run when the exported target was `motionkit::motionkit_core` but consumers used `motionkit::core` |
 
 ---
 
@@ -176,6 +181,12 @@ Unit tests assert known values; the interesting ones assert **properties** over
 thousands of uniformly sampled rotations from a fixed seed — a property test you
 cannot replay is a flake, not a test.
 
+Four allocation tests are instrumentation rather than ordinary unit tests. They
+run in their own executable because their global `operator new`/`operator delete`
+replacements affect an entire process. That target alone suppresses GNU's
+`-Wmismatched-new-delete` diagnostic: the `malloc`/`free` pairing is deliberate
+and is the mechanism being tested. The warning remains enabled everywhere else.
+
 - **Group axioms**: associativity, inverse, composition matching matrix product
 - **Invariants**: stored quaternion is always unit and canonical; `matrix()` is always in SO(3)
 - **Round trips**: quaternion ↔ matrix ↔ rotation vector ↔ RPY
@@ -188,4 +199,4 @@ cannot replay is a flake, not a test.
 
 ## Licence
 
-Apache-2.0.
+Apache-2.0.

docs/adr/0004-verify-the-installed-package-in-ci.md

@@ -36,17 +36,32 @@ project weeks later, where it would have looked like the consumer's fault.
 
 CI runs an `install-consumer` job that:
 
-1. Builds and installs the package into a staging prefix.
+1. Configures with `MOTIONKIT_BUILD_TESTS=OFF`, then builds and installs the
+   package into a staging prefix.
 2. Generates a fresh, minimal CMake project that has never seen the source tree.
 3. Configures it with `CMAKE_PREFIX_PATH` pointing at the staging prefix, using
    only `find_package(motionkit REQUIRED)` and the public target name.
 4. Compiles it, **and runs the resulting binary**.
 
 Step 4 matters: a package can configure and link correctly and still ship
 headers that do not match the installed library.
+The consumer uses explicit failure return codes rather than `assert`, because
+the Release build disables assertions.
 
-The consumer source deliberately uses the exact snippet published in the
-README. If the README example stops working, CI fails.
+The consumer deliberately uses the public include and CMake target published in
+the README. If the documented `find_package` and link contract stops working,
+CI fails.
+
+The consumer does not set `CMAKE_CXX_STANDARD`, and it includes the frame-graph
+API whose defaulted comparison requires C++20. The installed `motionkit::core`
+target propagates its `cxx_std_20` requirement, so the package must put a
+downstream compiler into a compatible language mode itself.
+
+Disabling the repository tests is part of the boundary, not an optimisation.
+Top-level motionkit builds enable tests by default, which would fetch GoogleTest
+and compile test-only instrumentation before the installed package is exercised.
+Those checks belong to the build-and-test jobs. The install-consumer job should
+fail only when building, installing or consuming the package fails.
 
 ## Consequences
 
@@ -56,6 +71,8 @@ README. If the README example stops working, CI fails.
 - Header install coverage is checked implicitly — a header omitted from
   `install(DIRECTORY ...)` fails this job and nothing else.
 - The documented usage example cannot silently rot.
+- Test-only dependencies and instrumentation cannot make the package-consumer
+  gate fail before it reaches the installed artifact.
 
 **Negative**
 

docs/adr/0005-frame-graph-is-a-tree.md

@@ -156,5 +156,21 @@ nothrow and sized forms pointing at the runtime's implementation, so memory
 obtained one way is released the other. AddressSanitizer catches it immediately —
 `alloc-dealloc-mismatch (operator new vs free)`, raised from inside GoogleTest's
 own `stable_sort`, which allocates a temporary buffer. All eight `new` forms and
-all twelve `delete` forms now route through `malloc`/`free` so the pairing holds
+all twelve `delete` forms now use matching allocation families (`malloc`/`free`,
+or `_aligned_malloc`/`_aligned_free` for aligned MSVC forms) so the pairing holds
 whichever form the standard library reaches for.
+
+Those replacements have executable-wide linkage, so the four allocation checks
+live in a dedicated test target rather than changing allocation for every unit
+test. The 73 ordinary tests remain in the main test executable.
+
+The allocation target is intentionally not built by the TSan preset. The Clang
+TSan runtime provides its own strong global allocation symbols, and linking a
+second set is a duplicate-symbol error before any test can run. TSan still covers
+the library and all 73 ordinary tests; only the incompatible instrumentation is
+excluded.
+
+GNU also diagnoses the deliberate custom `new`/`delete` plumbing as
+`-Wmismatched-new-delete`. That warning is suppressed only for the dedicated
+allocation target. It remains enabled, and promoted to an error, for the library
+and every ordinary test target.

include/motionkit/core/frame_graph.hpp

@@ -1,6 +1,7 @@
 // SPDX-License-Identifier: Apache-2.0
 #pragma once
 
+#include <cstddef>
 #include <cstdint>
 #include <string>
 #include <string_view>
@@ -49,7 +50,9 @@ struct Expected {
   FrameError error{FrameError::None};
 
   constexpr explicit operator bool() const noexcept { return error == FrameError::None; }
-  constexpr bool hasValue() const noexcept { return error == FrameError::None; }
+  [[nodiscard]] constexpr bool hasValue() const noexcept {
+    return error == FrameError::None;
+  }
 };
 
 /// Opaque handle to a frame in one FrameGraph.
@@ -61,15 +64,15 @@ class FrameId {
  public:
   constexpr FrameId() noexcept = default;
 
-  constexpr bool valid() const noexcept { return index_ != kInvalidIndex; }
+  [[nodiscard]] constexpr bool valid() const noexcept { return index_ != kInvalidIndex; }
   constexpr bool operator==(const FrameId& other) const noexcept = default;
 
   /// Index into the owning graph, for diagnostics only.
-  constexpr std::uint32_t index() const noexcept { return index_; }
+  [[nodiscard]] constexpr std::uint32_t index() const noexcept { return index_; }
 
  private:
   friend class FrameGraph;
-  static constexpr std::uint32_t kInvalidIndex = 0xFFFFFFFFu;
+  static constexpr std::uint32_t kInvalidIndex = 0xFFFFFFFFU;
 
   explicit constexpr FrameId(std::uint32_t index) noexcept : index_(index) {}
 
@@ -127,7 +130,7 @@ class FrameGraph {
   FrameError setTransform(FrameId frame, const SE3& parent_T_frame) noexcept;
 
   /// The transform from `frame` to its parent.
-  Expected<SE3> transformToParent(FrameId frame) const noexcept;
+  [[nodiscard]] Expected<SE3> transformToParent(FrameId frame) const noexcept;
 
   /// Returns `a_T_b`: the pose of frame `b` expressed in frame `a`.
   ///
@@ -151,33 +154,33 @@ class FrameGraph {
   /// and `FrameGraphRealtime.LowestCommonAncestorBeatsRoutingThroughTheRoot`.
   ///
   /// Allocation-free and non-throwing.
-  Expected<SE3> lookup(FrameId a, FrameId b) const noexcept;
+  [[nodiscard]] Expected<SE3> lookup(FrameId a, FrameId b) const noexcept;
 
   /// Finds a frame by name. Linear scan: intended for setup and diagnostics,
   /// not for the hot path. Resolve names to FrameIds once and keep the handles.
-  Expected<FrameId> find(std::string_view name) const noexcept;
+  [[nodiscard]] Expected<FrameId> find(std::string_view name) const noexcept;
 
   /// Name of a frame, or an empty view for an unknown handle.
-  std::string_view name(FrameId frame) const noexcept;
+  [[nodiscard]] std::string_view name(FrameId frame) const noexcept;
 
   /// Parent of `frame`, or an invalid FrameId if it is a root.
-  FrameId parent(FrameId frame) const noexcept;
+  [[nodiscard]] FrameId parent(FrameId frame) const noexcept;
 
   /// Number of ancestors between `frame` and its root; a root has depth 0.
-  Expected<std::uint32_t> depth(FrameId frame) const noexcept;
+  [[nodiscard]] Expected<std::uint32_t> depth(FrameId frame) const noexcept;
 
-  std::size_t size() const noexcept { return nodes_.size(); }
-  bool empty() const noexcept { return nodes_.empty(); }
+  [[nodiscard]] std::size_t size() const noexcept { return nodes_.size(); }
+  [[nodiscard]] bool empty() const noexcept { return nodes_.empty(); }
 
  private:
   struct Node {
     std::string name;
-    FrameId parent{};
-    SE3 parent_T_this{};
+    FrameId parent;
+    SE3 parent_T_this;
     std::uint32_t depth{0};
   };
 
-  bool isKnown(FrameId frame) const noexcept {
+  [[nodiscard]] bool isKnown(FrameId frame) const noexcept {
     return frame.valid() && frame.index_ < nodes_.size();
   }
 

include/motionkit/core/se3.hpp

@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: Apache-2.0
 #pragma once
 
+#include <array>
+
 #include "motionkit/core/so3.hpp"
 #include "motionkit/core/types.hpp"
 
@@ -19,30 +21,31 @@ class SE3 {
   SE3(const SO3& rotation, const Vec3& translation) noexcept
       : r_(rotation), t_(translation) {}
 
-  static SE3 fromTranslation(const Vec3& t) noexcept { return SE3(SO3{}, t); }
-  static SE3 fromRotation(const SO3& r) noexcept { return SE3(r, Vec3{}); }
+  static SE3 fromTranslation(const Vec3& t) noexcept { return {SO3{}, t}; }
+  static SE3 fromRotation(const SO3& r) noexcept { return {r, Vec3{}}; }
 
-  const SO3& rotation() const noexcept { return r_; }
-  const Vec3& translation() const noexcept { return t_; }
+  [[nodiscard]] const SO3& rotation() const noexcept { return r_; }
+  [[nodiscard]] const Vec3& translation() const noexcept { return t_; }
 
-  SE3 inverse() const noexcept;
+  [[nodiscard]] SE3 inverse() const noexcept;
   SE3 operator*(const SE3& rhs) const noexcept;
 
   /// Transforms a point: rotate, then translate.
   Vec3 operator*(const Vec3& point) const noexcept;
 
   /// Transforms a free vector (a direction or velocity): rotation only.
-  Vec3 rotateVector(const Vec3& v) const noexcept { return r_ * v; }
+  [[nodiscard]] Vec3 rotateVector(const Vec3& v) const noexcept { return r_ * v; }
 
   /// Row-major 4x4 homogeneous matrix, for interop with CAD and vision stacks
   /// that speak matrices rather than quaternions.
-  std::array<Scalar, 16> matrix() const noexcept;
+  [[nodiscard]] std::array<Scalar, 16> matrix() const noexcept;
 
-  bool isApprox(const SE3& other, Scalar linear_tol, Scalar angular_tol) const noexcept;
+  [[nodiscard]] bool isApprox(const SE3& other, Scalar linear_tol,
+                              Scalar angular_tol) const noexcept;
 
  private:
-  SO3 r_{};
-  Vec3 t_{};
+  SO3 r_;
+  Vec3 t_;
 };
 
 }  // namespace motionkit