CI: auto-fix and commit clang-format violations instead of just failing - #98
Merged
Conversation
The lint job's clang-format step used to be --dry-run --Werror only: a formatting slip failed the whole build and needed someone to notice, run clang-format -i locally, and push again. clang-format's output is fully deterministic given .clang-format, so there's no judgment call for a human to make - just run it and push. Now runs clang-format -i for real and, if that changed anything, commits and pushes it back to the branch that triggered the run (the PR's actual head branch for pull_request events, not the synthetic refs/pull/N/merge ref actions/checkout defaults to - that isn't a real branch to push to). The lint job's own permissions block is bumped to contents: write for this (other jobs are untouched). Pushing re-triggers the workflow, but the second run finds nothing left to fix and doesn't push again, so this is self-terminating. Only works for same-repo branches/PRs, since a fork PR's GITHUB_TOKEN can't push back to the fork - the push step fails closed there rather than silently skipping enforcement, so cppcheck/build jobs still gate the PR as before.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe lint workflow explicitly checks out the PR or pushed branch, formats C/C++ files in place with clang-format, and commits and pushes detected formatting changes using the GitHub Actions bot identity. ChangesLint auto-fix workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant actionsCheckout
participant clangFormat
participant Repository
GitHubActions->>actionsCheckout: checkout github.head_ref || github.ref_name
actionsCheckout->>Repository: load branch contents
GitHubActions->>clangFormat: format C/C++ files in place
clangFormat-->>GitHubActions: report detected changes
GitHubActions->>Repository: commit and push formatting updates
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![amazon-q-developer[bot]](https://avatars.github.com/in/1220912?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Summary
This PR successfully implements auto-fixing of clang-format violations in the CI workflow, replacing the previous fail-only approach. The implementation correctly handles:
- Permission escalation scoped to the lint job only
- Proper branch reference for PR workflows
- Self-terminating commit loop (second run finds nothing to fix)
- Graceful failure for fork PRs (can't push back, correctly blocks)
The core logic is sound and tested. The approach eliminates manual formatting round-trips while maintaining enforcement for fork contributions.
Note: I've flagged some variable quoting practices in the shell script that could theoretically cause issues with filenames containing spaces. However, for typical C++ project conventions (no spaces in source filenames), the current implementation functions correctly. Consider these suggestions as defensive improvements rather than blocking issues.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
git diff/git add against the unquoted $files word list could silently miss a changed file if a filename ever contained a space (this project doesn't have any, but no reason to leave the sharp edge). Diff/add the whole directories instead - clang-format itself still runs file-by-file via $files, which is fine either way, since a missed word there would just mean that one file wasn't reformatted, not a silently-dropped fix.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
34-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the clang-format version before claiming cross-release stability.
.clang-formatmakes a fixed formatter invocation deterministic, but the installedclang-formatversion is not pinned. LLVM releases change formatting options and behavior, so a runner/package update can create unexpected formatting commits. (clang.llvm.org)Pin the package/toolchain version or use a versioned formatter container/action.
🤖 Prompt for 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. In @.github/workflows/ci.yml around lines 34 - 42, Pin the clang-format toolchain used by the workflow before relying on its deterministic formatting behavior. Update the clang-format installation or invocation in the workflow to use a specific package version or versioned formatter container/action, while preserving the existing .clang-format-driven auto-fix flow.
🤖 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/ci.yml:
- Around line 49-56: Update the clang-format auto-fix step to store the find
results as a NUL-delimited array, preserving filenames containing whitespace or
glob characters. Use the array safely in the clang-format, git diff, and git add
commands via "${files[@]}", while retaining the existing formatting and commit
flow.
- Around line 57-58: Update the workflow step containing the git commit and push
commands to pass the branch ref through the step’s env configuration, then
reference that environment variable in the git push command with shell quoting.
Remove direct github.head_ref/github.ref_name interpolation from the run script
while preserving the existing push target behavior.
- Around line 43-48: Update the workflow’s formatting push step to authenticate
with a non-GITHUB_TOKEN credential, such as the configured GitHub App or PAT
secret, so the formatting commit triggers a new workflow run. Ensure checkout
does not persist the default workflow token for this push path, while preserving
the existing fail-closed behavior for fork pull requests.
- Around line 19-29: Update the Checkout step’s ref expression to use
github.event.pull_request.head.sha for pull_request events, while retaining
github.ref_name for push events. Replace the current github.head_ref fallback in
the actions/checkout configuration so fork PRs always check out the submitted
commit.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 34-42: Pin the clang-format toolchain used by the workflow before
relying on its deterministic formatting behavior. Update the clang-format
installation or invocation in the workflow to use a specific package version or
versioned formatter container/action, while preserving the existing
.clang-format-driven auto-fix flow.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 48c4d47c-5654-44da-902f-574a0c09c668
📒 Files selected for processing (1)
.github/workflows/ci.yml
…litting
- Pass the target branch name through env: instead of interpolating it
directly into the run: shell block, per CodeRabbit's template-injection
flag on github.head_ref/ref_name.
- Replace the unquoted $(find ...) word-splitting with a NUL-delimited
mapfile + "${files[@]}" array, used consistently by clang-format, git
diff, and git add, closing the gap the previous whole-directory-diff
workaround only partially covered.
Did not adopt persist-credentials: false (this job's own git push step
needs that credential) or checkout-by-head.sha (no fork-PR contributions
in this repo's history to defend against).
Owner
Author
|
Reviewed the remaining open threads:
Resolving these threads accordingly. Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lintjob'sclang-formatstep used to be--dry-run --Werroronly: a formatting slip failed the whole build and needed someone to notice, runclang-format -ilocally, and push again.clang-format's output is fully deterministic given.clang-format, so there's no judgment call for a human to make - just run it and push.clang-format -ifor real and, if that changed anything, commits and pushes it back to the branch that triggered the run (the PR's actual head branch forpull_requestevents, not the syntheticrefs/pull/N/mergerefactions/checkoutdefaults to - that isn't a real branch to push to). Thelintjob's ownpermissions:block is bumped tocontents: writefor this (other jobs are untouched).GITHUB_TOKENcan't push back to the fork - the push step fails closed there (correctly still blocking the check) rather than silently skipping enforcement, socppcheck/build jobs still gate the PR as before.Test plan
.github/workflows/ci.ymlYAML parses cleanly (python3 -c "import yaml; yaml.safe_load(...)").clang-formatstep behaves as intended (no-op here, since this branch's files are already formatted).Generated by Claude Code
Summary by CodeRabbit