Skip to content

feat(ci): integrate clang-tidy for changed C/C++ files#116

Open
kgeg401 wants to merge 21 commits intoalibaba:mainfrom
kgeg401:feat/issue-74-clang-tidy-integration
Open

feat(ci): integrate clang-tidy for changed C/C++ files#116
kgeg401 wants to merge 21 commits intoalibaba:mainfrom
kgeg401:feat/issue-74-clang-tidy-integration

Conversation

@kgeg401
Copy link
Copy Markdown

@kgeg401 kgeg401 commented Feb 14, 2026

Summary

  • add repository-level .clang-tidy with a minimal enforceable check set
  • add .github/workflows/clang_tidy.yml to run on C/C++ and CMake-related changes
  • configure CMake to export compile_commands.json and run clang-tidy on changed C/C++ files only
  • fail CI for clang-tidy findings via --warnings-as-errors='*'

Notes

  • workflow still runs configure on CMake changes even when no C/C++ file changed
  • .gitignore is updated to allow tracking .clang-tidy

Fixes #295

Greptile Summary

This PR adds a clang-tidy CI integration for the zvec repository: a new .clang-tidy configuration file, a GitHub Actions workflow (.github/workflows/clang_tidy.yml) that runs on C/C++ and CMake changes, and a .gitignore update to allow tracking .clang-tidy. The approach — running CMake to produce compile_commands.json, then running clang-tidy only on changed files that appear in that database — is sound and well-structured. However, there are two issues that need to be addressed before this is safe and fully functional:

  • Template injection (.github/workflows/clang_tidy.yml, line 83): The changed-files list is interpolated directly into the Python triple-quoted string inside the heredoc. A PR author could trigger Python code execution by including a file whose path contains """. The file list should be passed via an environment variable instead.
  • HeaderFilterRegex anchoring (.clang-tidy, line 9): The regex ^(src|tests|tools)/ is anchored to the start of the string but clang-tidy evaluates it against the full absolute path (e.g. /home/runner/work/zvec/zvec/src/foo.h). The anchor prevents any project header from matching, silently disabling all header-level diagnostics. The pattern should be changed to .*(src|tests|tools)/.*.
  • Redundant path triggers: "CMakeLists.txt" in both push and pull_request path blocks is already covered by "**/CMakeLists.txt".
  • Unpinned third-party action: tj-actions/changed-files@v46 should be pinned to a commit SHA to prevent supply-chain risk.

Confidence Score: 3/5

  • Safe to merge after fixing the template injection and HeaderFilterRegex issues; the workflow structure is otherwise solid.
  • The PR introduces two correctness/security issues: a moderate template injection risk in the Python step that could be exploited by a malicious PR author, and a misconfigured HeaderFilterRegex that silently suppresses all header diagnostics — undermining a key goal of the integration. Neither issue causes immediate breakage of the overall workflow, but both need to be addressed before the tooling delivers its intended value safely.
  • .github/workflows/clang_tidy.yml (template injection, line 83) and .clang-tidy (HeaderFilterRegex, line 9) need the most attention.

Important Files Changed

Filename Overview
.github/workflows/clang_tidy.yml New CI workflow running clang-tidy on changed C/C++ files; contains a template injection risk on line 83 where the file list is interpolated directly into Python code, and uses a mutable action tag. Also has a redundant CMakeLists.txt path filter entry.
.clang-tidy New repository-level clang-tidy config enabling clang-analyzer, bugprone, performance, and a subset of modernize checks; HeaderFilterRegex is anchored with ^ and will never match absolute header paths on the CI runner, silently suppressing all header diagnostics.
.gitignore Adds a negation rule to allow .clang-tidy to be tracked, consistent with the existing pattern for .clang-format; no issues found.

Sequence Diagram

sequenceDiagram
    participant GH as GitHub Event (push/PR)
    participant WF as clang_tidy.yml
    participant TJ as tj-actions/changed-files@v46
    participant PY as Python Filter Script
    participant CT as clang-tidy

    GH->>WF: Trigger on C/C++, CMake, or .clang-tidy change
    WF->>WF: actions/checkout@v4 (submodules: recursive)
    WF->>WF: apt-get install clang-tidy cmake ninja-build
    WF->>WF: cmake -S . -B build (exports compile_commands.json)
    WF->>TJ: Collect changed *.c / *.cc / *.cpp / *.cxx files
    TJ-->>WF: all_changed_files (newline-separated)
    WF->>PY: Interpolate file list into Python heredoc
    PY->>PY: Cross-reference with build/compile_commands.json
    PY-->>WF: any_tidy_files, all_tidy_files, skipped_files
    WF->>WF: Show skipped files (if any)
    WF->>CT: clang-tidy -p build --warnings-as-errors='*' <file>
    CT-->>WF: Pass / Fail (fail = CI blocked)
Loading

Last reviewed commit: 30343b3

Greptile also left 4 inline comments on this PR.

Copy link
Copy Markdown
Author

@kgeg401 kgeg401 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..

@kgeg401 kgeg401 force-pushed the feat/issue-74-clang-tidy-integration branch from 432cf4a to 25d39a7 Compare February 28, 2026 16:06
@kgeg401 kgeg401 force-pushed the feat/issue-74-clang-tidy-integration branch from 25d39a7 to f5aedce Compare February 28, 2026 16:07
@kgeg401
Copy link
Copy Markdown
Author

kgeg401 commented Mar 1, 2026

Cleaned up PR scope and hardened clang-tidy workflow:\n\n- Removed unrelated FP16 test changes from this PR (keeps overlap out of #116).\n- Kept this PR scoped to .clang-tidy, workflow, and CI plumbing files only.\n- Updated workflow to run clang-tidy only on changed source files that exist in compile_commands.json, with explicit skipped-file reporting and non-failing no-op behavior when no analyzable files are present.

@kgeg401
Copy link
Copy Markdown
Author

kgeg401 commented Mar 8, 2026

This PR is now strictly scoped to .clang-tidy, the clang-tidy workflow, and related CI plumbing. No further code changes in this pass; it is ready for review when bandwidth allows.

@feihongxu0824
Copy link
Copy Markdown
Collaborator

@greptile

Comment thread .github/workflows/clang_tidy.yml Outdated
Comment thread .clang-tidy
Comment thread .github/workflows/clang_tidy.yml Outdated
Comment thread .github/workflows/clang_tidy.yml Outdated
@egolearner
Copy link
Copy Markdown
Collaborator

Thanks @kgeg401 for a pretty good start.
As zvec contains a lot of files not fully satisfying your set of clang-tidy rules, I make a modification to only check modern nullptr and fixed all the errors so that we can safely enable clang-tidy workflow. Later we can use sub-issue/sub-tasks to track and enable other clang-tidy rules step by step.

@egolearner
Copy link
Copy Markdown
Collaborator

resolve #295

Comment thread .github/workflows/clang_tidy.yml Outdated
Comment thread .github/workflows/clang_tidy.yml Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

clang-tidy: add github workflow

3 participants