feat: add auto-fix option for automatic clang-format fixing - #443
shenxianpeng wants to merge 25 commits into
Conversation
github-actions
Bot
added
bug
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
b4670df to
ff93734
Compare
ff93734 to
9b8d058
Compare
Cpp-Linter Report
|
There was a problem hiding this comment.
Pull request overview
Adds an auto-fix capability to the cpp-linter composite action so clang-format issues can be automatically fixed, committed, and pushed back to the PR branch, reducing the manual “fix + repush” workflow.
Changes:
- Introduces new action inputs
auto-fixandauto-fix-commit-msg, and conditionally passes--fixtocpp-linter. - Adds an auto-commit + push step to publish formatting fixes back to the PR branch.
- Updates documentation and adds a new example workflow demonstrating auto-fix usage and required permissions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Documents the new auto-fix feature with usage and [skip ci] guidance. |
| docs/permissions.md | Documents the additional contents: write permission required for auto-fix. |
| docs/examples/index.md | Adds an auto-fix recipe entry linking to the new example workflow. |
| docs/action.yml | Adds metadata for new inputs (min version + required permissions). |
| action.yml | Implements auto-fix inputs, conditional --fix flag, and auto-commit/push logic. |
| .github/workflows/examples/auto-fix.yml | New example workflow showing required permissions and configuration for auto-fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
640601d to
c9e0ea4
Compare
fdfa0db to
e3fd91b
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughAdds configurable clang-format auto-fixing to the action. The action supports pull request branch checkout, conditional ChangesClang-format auto-fix
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/examples/auto-fix.yml:
- Around line 10-12: Remove the pull-requests: write permission from the
workflow permissions block, retaining contents: write for auto-fix commits. Keep
this example’s baseline permissions limited to the access it uses.
In `@action.yml`:
- Around line 539-553: Update the auto-fix flow around has_changes and git add
-A to avoid staging the entire workspace. Capture the pre-linter tracked-file
baseline and determine which paths were modified by formatting, then stage only
those formatter-produced paths before committing; alternatively require a clean
tree before running the formatter. Preserve unrelated generated or pre-existing
workspace changes.
- Around line 560-561: Update the auto-commit flow around branch and push_result
to run only for branch refs and pull-request runs, skipping tag-triggered
executions. Detect the ref type before constructing the destination branch, and
preserve the existing branch selection and push behavior for supported runs.
- Around line 460-468: Gate the auto-fix branch checkout and all subsequent
auto-fix commit/push behavior on the PR originating from the same repository as
the workflow repository, using the repository identity fields available in the
GitHub context. For fork pull requests, preserve the initial merge checkout,
skip the GITHUB_HEAD_REF fetch/checkout and auto-fix push path, and emit a
warning that auto-fix cannot update the fork’s head ref. Update the checkout
step and the nearby auto-fix commit/push logic rather than changing unrelated
behavior.
In `@docs/permissions.md`:
- Around line 87-90: Convert the YAML example under the permissions section to
the repository’s required indented code-block style by removing the fenced block
markers and indenting the example consistently. Preserve the existing
permissions contents unchanged.
- Around line 94-109: Update the CI re-triggering guidance near the auto-fix
commit message reference so [skip ci] is recommended only when pushing with a
PAT or GitHub App token; remove the implication that it is needed with the
default GITHUB_TOKEN, while preserving the existing token and auto-fix context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 144d5494-4fd2-4310-ab8c-50e1e5426494
📒 Files selected for processing (6)
.github/workflows/examples/auto-fix.ymlREADME.mdaction.ymldocs/action.ymldocs/examples/index.mddocs/permissions.md
Address review feedback on the auto-fix flow: - Gate the PR-branch checkout and the auto-commit/push steps to same-repository pull requests. Fork PRs previously hard-failed the checkout (the fork's branch isn't on origin) and could not push; now they are skipped with a warning. - Guard against tag refs: compute the destination branch from the PR head ref or a pushed branch ref, and skip otherwise so we never push HEAD to refs/heads/<tag> and create a stray branch. - Stage only tracked modifications with `git add -u` instead of `git add -A`, so unrelated untracked/generated files are not swept into the auto-fix commit. - docs/permissions.md: clarify the write permission is for the actions/checkout token, and correct the `[skip ci]` guidance (it only matters for PAT/App-token pushes, since the default GITHUB_TOKEN push does not trigger CI anyway). - Example workflow: downgrade `pull-requests: write` to `read` (the example uses no review/thread-comment feature; `read` is still needed for files-changed-only on pull_request events).
I would like to change the default clang-format commit message and leave "refactor: apply clang-tidy fixes" for clang-tidy in the futhure if we also support auto-fix for it Co-authored-by: Xianpeng Shen <xianpeng.shen@gmail.com>
Two places said something the implementation does not do. The README told everyone to tag the commit with `[skip ci]` to stop it re-running CI, but the example above it checks out with the default `GITHUB_TOKEN` -- which cannot start a workflow run at all, so there was nothing to skip. docs/permissions.md already had this right and the two contradicted each other. The tip now names the token that makes `[skip ci]` meaningful and points at the permissions page for both setups. `auto-fix`'s own description implied it rewrites whole files. It does not: cpp-linter assembles range-aware args, so `lines-changed-only` narrows what gets reformatted. That matters, because with it enabled a file can come back from auto-fix still failing a whole-file `.clang-format` check -- worth saying out loud rather than leaving to be discovered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- permissions.md: new "GitHub App token" section (App permissions, create-github-app-token, pass the token to checkout and cpp-linter); the CI re-trigger note points at it instead of recommending a PAT - fork note: fork pull requests get no secrets, so an App token or PAT cannot help there; auto-fix stays skipped - README tip and example workflow updated to match - docs/action.yml: auto-fix inputs land in 2.22.0, not 2.19.0
Co-authored-by: Brendan <2bndy5@gmail.com>
Give the GitHub App token setup its own section under Usage instead of only a passing mention in the auto-fix tip -- it applies to every feature and needs no server to host webhooks. Also add the relative link refs for permissions-doc and app-token-doc to docs/index.md, which includes the README; without them those links rendered as literal text on the docs homepage.
Co-authored-by: Brendan <2bndy5@gmail.com>
Co-authored-by: Xianpeng Shen <xianpeng.shen@gmail.com>
Re-applies the action.yml part of 8580162, which was dropped when the branch was rewritten. - On pull_request events check out the PR head commit only when HEAD is the merge commit, without --force; a dirty tree or a failed checkout skips auto-fix with a warning instead (actions/checkout provides refs/pull/N/merge, so committing there would push that merge into the branch) - Stage only modified files matching the configured `extensions` and count `git ls-files --modified` output instead of parsing an exit code
…kflow
On pull_request events actions/checkout provides refs/pull/N/merge, so
the action used to switch the workspace to the head commit itself. That
silently changes the tree every later step sees. Drop the step and make
the workflow responsible instead, the way git-auto-commit-action does:
document `ref: ${{ github.event.pull_request.head.sha }}` on
actions/checkout (head.sha rather than head_ref so fork PRs still check
out), and have the commit step verify HEAD == head.sha on pull_request,
skipping with a warning that names the missing input otherwise.
eebbabe to
d6664a0
Compare
…docs - one auto-fix step instead of two: the fork check is an early exit in the commit step, so the condition logic lives in one place - push failures print stderr plus the permissions hint instead of guessing the cause from the message text - shorter input description; README no longer restates it as a list; the two permissions-page warnings are one admonition
Adds a job that runs the action with auto-fix on the demo sources and asserts the resulting commit: message, author and email from the inputs, only files under docs/examples/demo, a clean tree afterwards and zero clang-format findings. The job has no push credentials, so the push is rejected by design and the last step checks the commit never reached the PR branch.
|
From the test PR cpp-linter/test-cpp-linter-action#61, it seems to work well. Do you have any other comments, feedback, or concerns about this PR @2bndy5 |
shenxianpeng
removed
bug
|
Does the test PR fail when the actions/checkout |
2bndy5
left a comment
2bndy5
left a comment
There was a problem hiding this comment.
A continuation of my other review comment and my observation in test repo.
Also need to restore the step that does git checkout; actions/checkout default ref value is insufficient for this feature.
|
We might want to also consider an additional input to filter the files in the commit. For projects trying to phase-in clang tooling, this |
…er staging Restore the step that switches from the pull_request merge commit to the head commit, so workflows can keep the default actions/checkout. It only acts when HEAD is the merge commit, never forces, and fetches the head commit only when it is missing: a --depth fetch truncates a full clone. Stage first and count from `git status --short`, as suggested in review. With a .gitattributes LF/CRLF rule the old check listed a file that `git add` normalized away, and `git commit` then failed the step. The pathspecs are resolved to modified tracked files first because `git add` aborts on a pathspec that matches nothing, and so that untracked sources are not committed.
Drop `ref` so the job covers the action's own checkout of the PR head, and skip the job on fork pull requests, where auto-fix makes no commit.
github-actions
Bot
added
bug
…request The fork guard in the commit step only fired when the event name was `pull_request`. A `pull_request_target` workflow that checks out the pull request head (a common setup to post comments on fork PRs) got past it: GITHUB_HEAD_REF is the fork's branch name, HEAD equals the head sha, and the step ran `git push origin HEAD:refs/heads/<fork branch>` against this repository with a token that has `contents: write`. From a fork's `main` that is a fast-forward of an unprotected `main`. Guard on the pull request payload instead: whenever the event carries a pull request whose head repository is not this repository (or is gone), skip with the existing warning. Push events have no payload and are unaffected.
cpp-linter changes into `repo-root` before it lints, but the two git steps ran in the workspace root. With the repository checked out in a subdirectory (`path:` on actions/checkout plus `repo-root`), `--fix` rewrote the files and nothing was committed: `git rev-parse HEAD` found no repository there, so the step reported "HEAD is , not the pull request head" on pull requests and "No formatting changes" on pushes. Change into `repo-root` first in both steps. The default `.` is a no-op.
shenxianpeng
removed
bug
|
@2bndy5 I've addressed your last review in c9db607 and b4e1034: the checkout step is restored, and it now stages first and counts from One difference from your snippet: I also pushed two small fixes (25f45eb, adbb9f0), explained in their commit messages. If it looks good, we might get this merged and handle the remaining edge cases as bug reports come in. Feel free to take your time to take a look. |
Closes #439
Adds an
auto-fixinput. When enabled, cpp-linter runs with--fix(clang-format-ion the files with style issues) and the action commits the result to the branch. Fixes respectlines-changed-only.Inputs
auto-fixfalseauto-fix-commit-msgstyle: apply clang-format fixesauto-fix-git-user$GITHUB_ACTORauto-fix-git-email$GITHUB_ACTOR_ID+$GITHUB_ACTOR@users.noreply.github.comHow it works
auto-fixis enabled. Pull requests from forks are skipped with a warning: the token cannot push to the fork's branch.pull_requestevents the default checkout is the merge commit, and a fix committed on it would carry that merge into the branch. The action therefore checks out the head commit itself before linting, so workflows keep the defaultactions/checkout. It does this only whenHEADis the merge commit, never forces, and fetches the head SHA only if it is missing, because a--depthfetch truncates a full clone. The commit step verifiesHEAD == head.shaand skips with a warning otherwise. Onpushevents it commits to the pushed branch; tags and other refs are skipped.extensionsinput, then counts what is staged withgit status --short. Files an earlier step changed and untracked sources stay out of the commit, and a.gitattributesLF/CRLF normalization cannot lead to an empty commit.HEAD:refs/heads/<branch>. A rejected push is a warning with the git error and the permissions hint, not a failure.Requires cpp-linter >= 1.14.0, which ships
--fix; the pin on this branch is 1.14.0.Docs
README.md: auto-fix section with a note that the action checks out the pull request head,[skip ci]tip, and a section on running the action with your own GitHub App tokendocs/permissions.md:contents: write, the same checkout note, the limits (defaultGITHUB_TOKENdoes not trigger CI; forks), and a "GitHub App token" chapterdocs/examples/index.md+.github/workflows/examples/auto-fix.yml: recipedocs/action.yml: minimum version 2.23.0 for the four inputsTests
self-test.ymlgains atest-auto-fixjob that runs the action with auto-fix on the demo sources and asserts the commit (message, author, only files underdocs/examples/demo, clean tree, zero remaining clang-format findings). It has no push credentials, so the push is rejected by design and the last step checks the commit never reached the PR branch. It uses the default checkout, so the action's own checkout of the pull request head is covered, and it is skipped on fork pull requests, where auto-fix makes no commit.Summary by CodeRabbit
New Features
Documentation
[skip ci]commit tags.