clang-tidy environmental consistency (#109)

* Consistent clang-tidy.sh script
* Fixes all remaining warnings
* Treat warnings as errors for CI purposes/not building tech debt

Author: alan-george-lk

.clang-tidy

@@ -9,6 +9,7 @@ Checks: >
   -bugprone-easily-swappable-parameters,
   -modernize-use-trailing-return-type,
   -modernize-avoid-c-arrays,
+  -modernize-type-traits,
   -modernize-use-auto,
   -modernize-use-nodiscard,
   -modernize-return-braced-init-list,
@@ -29,7 +30,8 @@ WarningsAsErrors: >
   bugprone-string-literal-with-embedded-nul,
   bugprone-suspicious-memset-usage,
 
-HeaderFilterRegex: '(include/livekit|src|examples)'
+HeaderFilterRegex: '.*/(include/livekit|src)/.*\.(h|hpp)$'
+ExcludeHeaderFilterRegex: '(.*/src/tests/.*)|(.*/_deps/.*)|(.*/build-[^/]*/.*)'
 
 FormatStyle: file
 

.github/workflows/builds.yml

@@ -553,7 +553,6 @@ jobs:
     continue-on-error: false
     permissions:
       contents: read
-      pull-requests: write
 
     steps:
       - name: Checkout (with submodules)
@@ -568,8 +567,28 @@ jobs:
           sudo apt-get update
           sudo apt-get install -y \
             build-essential cmake ninja-build pkg-config \
-            llvm-dev libclang-dev clang clang-tidy \
-            libssl-dev
+            llvm-dev libclang-dev clang \
+            libssl-dev wget ca-certificates gnupg
+
+      - name: Install clang-tidy 19 (for ExcludeHeaderFilterRegex support)
+        run: |
+          set -eux
+          # Ubuntu 24.04 apt ships clang-tidy 18, which doesn't understand
+          # ExcludeHeaderFilterRegex (added in 19). Pull clang-tidy 19 from
+          # the upstream LLVM apt repository and pin the unversioned names.
+          sudo install -m 0755 -d /etc/apt/keyrings
+          wget -qO- https://apt.llvm.org/llvm-snapshot.gpg.key \
+            | sudo tee /etc/apt/keyrings/llvm.asc >/dev/null
+          sudo chmod a+r /etc/apt/keyrings/llvm.asc
+          codename=$(lsb_release -cs)
+          echo "deb [signed-by=/etc/apt/keyrings/llvm.asc] http://apt.llvm.org/${codename}/ llvm-toolchain-${codename}-19 main" \
+            | sudo tee /etc/apt/sources.list.d/llvm-19.list >/dev/null
+          sudo apt-get update
+          sudo apt-get install -y clang-tidy-19 clang-tools-19
+          sudo ln -sf /usr/bin/clang-tidy-19 /usr/local/bin/clang-tidy
+          sudo ln -sf /usr/bin/run-clang-tidy-19 /usr/local/bin/run-clang-tidy
+          clang-tidy --version
+          run-clang-tidy --help | head -1 || true
 
       - name: Install Rust (stable)
         uses: dtolnay/rust-toolchain@3c5f7ea28cd621ae0bf5283f0e981fb97b8a7af9
@@ -590,33 +609,9 @@ jobs:
         run: cmake --build build-release --target livekit_proto
 
       - name: Run clang-tidy
-        uses: cpp-linter/cpp-linter-action@77c390c5ba9c947ebc185a3e49cc754f1558abb5 # v2.18.0
-        id: linter
         env:
-          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-        with:
-          style: ''
-          tidy-checks: ''
-          database: build-release
-          files-changed-only: false
-          lines-changed-only: true
-          extensions: 'c,cpp,cc,cxx'
-          ignore: 'build-*|cpp-example-collection|client-sdk-rust|vcpkg_installed|src/tests|bridge|src/room_event_converter.cpp'
-          file-annotations: true
-          thread-comments: false
-          step-summary: true
-          tidy-review: false
-          no-lgtm: true
-          jobs: 0   # 0 == use all available CPU cores
-
-      - name: Check warning thresholds
-        env:
-          TIDY_FINDINGS: ${{ steps.linter.outputs.clang-tidy-checks-failed }}
-          MAX_TIDY_FINDINGS: '0'
-        run: |
-          echo "clang-tidy findings: ${TIDY_FINDINGS}"
-          if [ "${TIDY_FINDINGS}" -gt "${MAX_TIDY_FINDINGS}" ]; then
-            echo "::warning::clang-tidy found ${TIDY_FINDINGS} issue(s), threshold is ${MAX_TIDY_FINDINGS}"
-            exit 1
-          fi
-          echo "clang-tidy findings within threshold"
+          TIDY_BLOB_SHA: ${{ github.event.pull_request.head.sha || github.sha }}
+        # This script is intended to be run locally and in CI. It will auto-detect
+        # the GHA environment and add PR annotations and a run summary.
+        # As of writing all warnings are treateded as errors to avoid tech debt build up
+        run: ./scripts/clang-tidy.sh --fail-on-warning

.gitignore

@@ -30,7 +30,7 @@ lib/
 *.dylib
 *.dll
 *.exe
-livekit.log
+*.log
 web/
 *trace.json
 compile_commands.json

AGENTS.md

@@ -79,6 +79,7 @@ Be sure to update the directory layout in this file if the directory layout chan
 | `client-sdk-rust/livekit-ffi/protocol/*.proto` | FFI contract (protobuf definitions, read-only reference) |
 | `cmake/` | Build helpers (`protobuf.cmake`, `spdlog.cmake`, `LiveKitConfig.cmake.in`) |
 | `docker/` | Dockerfile for CI and SDK distribution images |
+| `scripts/` | Developer / CI helper scripts (e.g. `clang-tidy.sh`) |
 | `.github/workflows/` | GitHub Actions CI workflows |
 
 ### Key Types

README.md

@@ -522,13 +522,28 @@ sudo apt-get install clang-format clang-tidy clang-tools
 
 To run:
 
-1. Run a build script command, such that `compile_command.json` is present at the root of the repository
-2. Run:
+1. Generate `compile_commands.json` and the protobuf headers via a release build:
 
 ```bash
-clang-tidy -p . src/*.cpp
+./build.sh release
 ```
 
+1. Run the clang-tidy wrapper, which uses the same file set, regex filters, and `.clang-tidy` config as CI:
+
+```bash
+./scripts/clang-tidy.sh
+```
+
+The wrapper forwards extra arguments to `run-clang-tidy`, examples below:
+
+```bash
+./scripts/clang-tidy.sh -j 4                                # Change number of cores used
+./scripts/clang-tidy.sh -checks='-*,misc-const-correctness' # Only run certain checks
+./scripts/clang-tidy.sh -fix                                # Apply fixes automatically
+```
+
+Output is captured to `clang-tidy.log` at the repo root. This is done as a convenience feature, as often times the terminal buffer is not large enough for all the output.
+
 ### Memory Checks
 
 Run valgrind on various examples or tests to check for memory leaks and other issues.

include/livekit/audio_stream.h

@@ -41,9 +41,13 @@ class FfiEvent;
  * This struct wraps an AudioFrame and is used as the output type when
  * reading from an AudioStream.
  */
+// NOLINTBEGIN(bugprone-exception-escape)
+// AudioFrame can throw in various places monitored by bugprone-exception-escape
+// Suppressing for now, would require significant refactor to fix
 struct AudioFrameEvent {
   AudioFrame frame; ///< The decoded PCM audio frame.
 };
+// NOLINTEND(bugprone-exception-escape)
 
 /**
  * Represents a pull-based stream of decoded PCM audio frames coming from

include/livekit/local_track_publication.h

@@ -24,16 +24,11 @@ namespace proto {
 class OwnedTrackPublication;
 }
 
-class Track;
-
 class LocalTrackPublication : public TrackPublication {
 public:
   /// Note, this LocalTrackPublication is constructed internally only;
   /// safe to accept proto::OwnedTrackPublication.
   explicit LocalTrackPublication(const proto::OwnedTrackPublication &owned);
-
-  /// Typed accessor for the attached LocalTrack (if any).
-  std::shared_ptr<Track> track() const noexcept;
 };
 
 } // namespace livekit

include/livekit/remote_track_publication.h

@@ -24,17 +24,12 @@ namespace proto {
 class OwnedTrackPublication;
 }
 
-class Track;
-
 class RemoteTrackPublication : public TrackPublication {
 public:
   /// Note, this RemoteTrackPublication is constructed internally only;
   /// safe to accept proto::OwnedTrackPublication.
   explicit RemoteTrackPublication(const proto::OwnedTrackPublication &owned);
-
-  /// Typed accessor for the attached RemoteTrack (if any).
-  std::shared_ptr<Track> track() const noexcept;
-
+  
   bool subscribed() const noexcept { return subscribed_; }
 
   void setSubscribed(bool subscribed);

include/livekit/result.h

@@ -64,49 +64,62 @@ template <typename T, typename E> class [[nodiscard]] Result {
   /// Allows `if (result)` style success checks.
   explicit operator bool() const noexcept { return ok(); }
 
+  // TODO (AEG): clang-tidy flagged these accessors because the signatures are marked
+  // noexcept, but std::get can throw a std::bad_variant_access exception on 
+  // std::variant specifically. Investigate if this is actually a concern or if the types
+  // are safe within this class (unit test ideal).
+
   /// Access the success value. Requires `ok() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   T &value() & noexcept {
     assert(ok());
     return std::get<0>(storage_);
   }
 
   /// Access the success value. Requires `ok() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   const T &value() const & noexcept {
     assert(ok());
     return std::get<0>(storage_);
   }
 
   /// Move the success value out. Requires `ok() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   T &&value() && noexcept {
     assert(ok());
     return std::get<0>(std::move(storage_));
   }
 
   /// Move the success value out. Requires `ok() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   const T &&value() const && noexcept {
     assert(ok());
     return std::get<0>(std::move(storage_));
   }
 
   /// Access the error value. Requires `has_error() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   E &error() & noexcept {
     assert(has_error());
     return std::get<1>(storage_);
   }
 
   /// Access the error value. Requires `has_error() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   const E &error() const & noexcept {
     assert(has_error());
     return std::get<1>(storage_);
   }
 
   /// Move the error value out. Requires `has_error() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   E &&error() && noexcept {
     assert(has_error());
     return std::get<1>(std::move(storage_));
   }
 
   /// Move the error value out. Requires `has_error() == true`.
+  // NOLINTNEXTLINE(bugprone-exception-escape)
   const E &&error() const && noexcept {
     assert(has_error());
     return std::get<1>(std::move(storage_));

include/livekit/subscription_thread_dispatcher.h

@@ -471,7 +471,7 @@ class SubscriptionThreadDispatcher {
       active_readers_;
 
   /// Next auto-increment ID for data frame callbacks.
-  DataFrameCallbackId next_data_callback_id_;
+  DataFrameCallbackId next_data_callback_id_{0};
 
   /// Registered data frame callbacks keyed by opaque callback ID.
   std::unordered_map<DataFrameCallbackId, RegisteredDataCallback>