Draft - clang-tidy environmental consistency

.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,17 @@ 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
+        # tidy.sh auto-detects $GITHUB_ACTIONS and emits ::warning/::error
+        # workflow commands so findings surface as PR file annotations.
+        # It exits non-zero only when a finding is promoted to an error via
+        # WarningsAsErrors in .clang-tidy; pure warnings annotate but don't
+        # fail the build.
         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"
+          # Linkify findings in the step summary against the PR head commit
+          # rather than GITHUB_SHA. On pull_request events GITHUB_SHA is the
+          # ephemeral refs/pull/N/merge commit, whose blob URLs 404 once the
+          # PR closes; the head SHA stays resolvable. For push /
+          # workflow_dispatch / schedule runs this expression resolves to
+          # github.sha, which is the pushed/selected commit.
+          TIDY_BLOB_SHA: ${{ github.event.pull_request.head.sha || github.sha }}
+        run: ./tidy.sh

.gitignore

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

src/audio_stream.cpp

@@ -55,7 +55,7 @@ AudioStream::fromParticipant(Participant &participant, TrackSource track_source,
 AudioStream::~AudioStream() { close(); }
 
 AudioStream::AudioStream(AudioStream &&other) noexcept {
-  std::lock_guard<std::mutex> lock(other.mutex_);
+  const std::scoped_lock<std::mutex> lock(other.mutex_);
   queue_ = std::move(other.queue_);
   capacity_ = other.capacity_;
   eof_ = other.eof_;
@@ -76,8 +76,8 @@ AudioStream &AudioStream::operator=(AudioStream &&other) noexcept {
   close();
 
   {
-    std::lock_guard<std::mutex> lock_this(mutex_);
-    std::lock_guard<std::mutex> lock_other(other.mutex_);
+    const std::scoped_lock<std::mutex> lock_this(mutex_);
+    const std::scoped_lock<std::mutex> lock_other(other.mutex_);
 
     queue_ = std::move(other.queue_);
     capacity_ = other.capacity_;
@@ -110,7 +110,7 @@ bool AudioStream::read(AudioFrameEvent &out_event) {
 
 void AudioStream::close() {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (closed_) {
       return;
     }
@@ -221,7 +221,7 @@ void AudioStream::onFfiEvent(const FfiEvent &event) {
 
 void AudioStream::pushFrame(AudioFrameEvent &&ev) {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
 
     if (closed_ || eof_) {
       return;
@@ -239,7 +239,7 @@ void AudioStream::pushFrame(AudioFrameEvent &&ev) {
 
 void AudioStream::pushEos() {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (eof_) {
       return;
     }

src/data_stream.cpp

@@ -38,7 +38,7 @@ std::string generateRandomId(std::size_t bytes = 16) {
   out.reserve(bytes * 2);
   const char *hex = "0123456789abcdef";
   for (std::size_t i = 0; i < bytes; ++i) {
-    int v = dist(rng);
+    const int v = dist(rng);
     out.push_back(hex[(v >> 4) & 0xF]);
     out.push_back(hex[v & 0xF]);
   }
@@ -109,7 +109,7 @@ TextStreamReader::TextStreamReader(TextStreamInfo info)
 
 void TextStreamReader::onChunkUpdate(const std::string &text) {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (closed_)
       return;
     queue_.push_back(text);
@@ -120,7 +120,7 @@ void TextStreamReader::onChunkUpdate(const std::string &text) {
 void TextStreamReader::onStreamClose(
     const std::map<std::string, std::string> &trailer_attrs) {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     for (const auto &kv : trailer_attrs) {
       info_.attributes[kv.first] = kv.second;
     }
@@ -154,7 +154,7 @@ ByteStreamReader::ByteStreamReader(ByteStreamInfo info)
 
 void ByteStreamReader::onChunkUpdate(const std::vector<std::uint8_t> &bytes) {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (closed_)
       return;
     queue_.push_back(bytes);
@@ -165,7 +165,7 @@ void ByteStreamReader::onChunkUpdate(const std::vector<std::uint8_t> &bytes) {
 void ByteStreamReader::onStreamClose(
     const std::map<std::string, std::string> &trailer_attrs) {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     for (const auto &kv : trailer_attrs) {
       info_.attributes[kv.first] = kv.second;
     }
@@ -308,13 +308,13 @@ TextStreamWriter::TextStreamWriter(
 }
 
 void TextStreamWriter::write(const std::string &text) {
-  std::lock_guard<std::mutex> lock(write_mutex_);
+  const std::scoped_lock<std::mutex> lock(write_mutex_);
   if (closed_)
     throw std::runtime_error("Cannot write to closed TextStreamWriter");
 
   for (const auto &chunk_str : splitUtf8(text, kStreamChunkSize)) {
     const auto *p = reinterpret_cast<const std::uint8_t *>(chunk_str.data());
-    std::vector<std::uint8_t> bytes(p, p + chunk_str.size());
+    const std::vector<std::uint8_t> bytes(p, p + chunk_str.size());
     LK_LOG_DEBUG("sending chunk");
     sendChunk(bytes);
   }
@@ -339,7 +339,7 @@ ByteStreamWriter::ByteStreamWriter(
 }
 
 void ByteStreamWriter::write(const std::vector<std::uint8_t> &data) {
-  std::lock_guard<std::mutex> lock(write_mutex_);
+  const std::scoped_lock<std::mutex> lock(write_mutex_);
   if (closed_)
     throw std::runtime_error("Cannot write to closed ByteStreamWriter");
 
@@ -348,7 +348,7 @@ void ByteStreamWriter::write(const std::vector<std::uint8_t> &data) {
     const std::size_t n =
         std::min<std::size_t>(kStreamChunkSize, data.size() - offset);
     auto it = data.begin() + static_cast<std::ptrdiff_t>(offset);
-    std::vector<std::uint8_t> chunk(it, it + static_cast<std::ptrdiff_t>(n));
+    const std::vector<std::uint8_t> chunk(it, it + static_cast<std::ptrdiff_t>(n));
     sendChunk(chunk);
     offset += n;
   }

src/data_track_stream.cpp

@@ -38,7 +38,7 @@ void DataTrackStream::init(FfiHandle subscription_handle) {
 
 bool DataTrackStream::read(DataTrackFrame &out) {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (closed_ || eof_) {
       return false;
     }
@@ -70,7 +70,7 @@ bool DataTrackStream::read(DataTrackFrame &out) {
 void DataTrackStream::close() {
   std::int32_t listener_id = -1;
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (closed_) {
       return;
     }
@@ -94,7 +94,7 @@ void DataTrackStream::onFfiEvent(const FfiEvent &event) {
 
   const auto &dts = event.data_track_stream_event();
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (closed_ || dts.stream_handle() !=
                        static_cast<std::uint64_t>(subscription_handle_.get())) {
       return;
@@ -111,7 +111,7 @@ void DataTrackStream::onFfiEvent(const FfiEvent &event) {
 }
 
 void DataTrackStream::pushFrame(DataTrackFrame &&frame) {
-  std::lock_guard<std::mutex> lock(mutex_);
+  const std::scoped_lock<std::mutex> lock(mutex_);
 
   if (closed_ || eof_) {
     return;
@@ -128,7 +128,7 @@ void DataTrackStream::pushFrame(DataTrackFrame &&frame) {
 
 void DataTrackStream::pushEos() {
   {
-    std::lock_guard<std::mutex> lock(mutex_);
+    const std::scoped_lock<std::mutex> lock(mutex_);
     if (eof_) {
       return;
     }

src/ffi_client.cpp

@@ -45,6 +45,25 @@
   throw std::runtime_error(error_msg);
 }
 
+// TEMP: INTENTIONAL clang-tidy triggers to demo how GitHub CI renders red
+// (error-level) annotations. Each line below hits a check promoted to
+// WarningsAsErrors in .clang-tidy. Revert this block before merging.
+// NOLINTBEGIN(misc-const-correctness)
+[[maybe_unused]] void debug_tidy_error_markers() {
+  std::string s = "hello";
+  std::string t = std::move(s);
+  (void)s.size(); // bugprone-use-after-move
+  (void)t;
+
+  int i = 0;
+  while (i < 10) { // bugprone-infinite-loop
+    (void)t;
+  }
+
+  (void)sizeof(sizeof(int)); // bugprone-sizeof-expression
+}
+// NOLINTEND(misc-const-correctness)
+
 std::optional<FfiClient::AsyncId> ExtractAsyncId(const proto::FfiEvent &event) {
   using E = proto::FfiEvent;
   switch (event.message_case()) {
@@ -172,14 +191,14 @@
 
 FfiClient::ListenerId
 FfiClient::AddListener(const FfiClient::Listener &listener) {
-  std::lock_guard<std::mutex> guard(lock_);
-  FfiClient::ListenerId id = next_listener_id++;
+  const std::scoped_lock<std::mutex> guard(lock_);
+  const FfiClient::ListenerId id = next_listener_id++;
   listeners_[id] = listener;
   return id;
 }
 
 void FfiClient::RemoveListener(ListenerId id) {
-  std::lock_guard<std::mutex> guard(lock_);
+  const std::scoped_lock<std::mutex> guard(lock_);
   listeners_.erase(id);
 }
 
@@ -191,7 +210,7 @@
   }
   const uint8_t *resp_ptr = nullptr;
   size_t resp_len = 0;
-  FfiHandleId handle =
+  const FfiHandleId handle =
       livekit_ffi_request(reinterpret_cast<const uint8_t *>(bytes.data()),
                           bytes.size(), &resp_ptr, &resp_len);
   if (handle == INVALID_HANDLE) {
@@ -216,7 +235,7 @@
   std::unique_ptr<PendingBase> to_complete;
   std::vector<Listener> listeners_copy;
   {
-    std::lock_guard<std::mutex> guard(lock_);
+    const std::scoped_lock<std::mutex> guard(lock_);
 
     // Complete pending future if this event is a callback with async_id
     if (auto async_id = ExtractAsyncId(event)) {
@@ -259,7 +278,7 @@
 bool FfiClient::cancelPendingByAsyncId(AsyncId async_id) {
   std::unique_ptr<PendingBase> to_cancel;
   {
-    std::lock_guard<std::mutex> guard(lock_);
+    const std::scoped_lock<std::mutex> guard(lock_);
     auto it = pending_by_id_.find(async_id);
     if (it != pending_by_id_.end()) {
       to_cancel = std::move(it->second);
@@ -283,7 +302,7 @@
   pending->match = std::move(match);
   pending->handler = std::move(handler);
   {
-    std::lock_guard<std::mutex> guard(lock_);
+    const std::scoped_lock<std::mutex> guard(lock_);
     pending_by_id_.emplace(async_id, std::move(pending));
   }
   return fut;
@@ -455,7 +474,7 @@
   get_stats_req->set_request_async_id(async_id);
 
   try {
-    proto::FfiResponse resp = sendRequest(req);
+    const proto::FfiResponse resp = sendRequest(req);
     if (!resp.has_get_stats()) {
       logAndThrow("FfiResponse missing get_stats");
     }
@@ -502,7 +521,7 @@
         }
 
         const proto::OwnedTrackPublication &pub = cb.publication();
-        pr.set_value(std::move(pub));
+        pr.set_value(pub);
       });
 
   // Build and send the request
@@ -701,7 +720,7 @@
   }
 
   try {
-    proto::FfiResponse resp = sendRequest(req);
+    const proto::FfiResponse resp = sendRequest(req);
     if (!resp.has_subscribe_data_track()) {
       return Result<proto::OwnedDataTrackStream,
                     SubscribeDataTrackError>::failure(SubscribeDataTrackError{