ci: convert clang-format diff check to informational PR comment#7251
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
PastaPastaPasta
left a comment
There was a problem hiding this comment.
utACK; the value of this check is imo low; and seeing CI "failing" causes agents to overreact. It's easier to fix the agent misbehavior by not having CI "fail" when it likely is ok.
24984ea to
2301131
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe GitHub Actions workflow Sequence Diagram(s)sequenceDiagram
participant Runner as GitHub Actions Runner
participant ClangScript as clang-format-diff.py
participant Git as Git (diff)
participant GH_API as GitHub API (find/create-comment)
Runner->>Git: fetch origin/develop, compute diff for PR
Runner->>ClangScript: run clang-format-diff.py -> diff_output.txt
ClangScript-->>Runner: return diff_output.txt
alt diff_output.txt non-empty
Runner->>Runner: set has_diff = true
Runner->>GH_API: build markdown with unified diff
GH_API->>GitHub: create-or-update PR comment with diff
GH_API-->>Runner: comment posted/updated
else diff_output.txt empty
Runner->>Runner: set has_diff = false
Runner->>GH_API: update existing comment -> "No formatting issues found"
GH_API-->>Runner: comment updated (if present)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
How about making it informational instead? |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean single-file deletion PR. The maintainer suggestion to convert to an informational check is real and worth discussing, but the documentation finding is a false positive — docs reference the local script (which is retained), not the CI workflow.
Reviewed commit: 2301131
🟡 1 suggestion(s)
1 additional finding
🟡 suggestion: Consider making the check informational instead of removing it
.github/workflows/clang-diff-format.yml (lines 1-24)
Verified: maintainer UdjinM6 commented on this PR suggesting converting the workflow to a non-blocking informational check that posts formatting diffs as PR comments, rather than removing it entirely. See their prototype at UdjinM6/dash@59084cb and example at UdjinM6#26. This is a legitimate design discussion point — an informational CI check is more discoverable than expecting contributors to run the local script manually.
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `.github/workflows/clang-diff-format.yml`:
- [SUGGESTION] lines 1-24: Consider making the check informational instead of removing it
Verified: maintainer UdjinM6 commented on this PR suggesting converting the workflow to a non-blocking informational check that posts formatting diffs as PR comments, rather than removing it entirely. See their prototype at UdjinM6/dash@59084cb and example at UdjinM6/dash#26. This is a legitimate design discussion point — an informational CI check is more discoverable than expecting contributors to run the local script manually.
I think that this would still result in agents etc seeing the comment and making changes to "fix" all of the differences. |
Will tweaking the message help? cda9829 @thepastaclaw @coderabbitai is it clear that you don't have to do anything when you see a message like that? Can it be improved? |
|
Tip For best results, initiate chat on the files or code changes.
The proposed message is reasonably clear to me. The phrase "no action required" combined with "not required to be fixed and will not block this PR" signals that this is purely informational. I would not flag these as review issues when the check clearly states no action is needed. That said, here are a few suggestions to make it even more robust against AI agents acting on it:
A combined suggestion: In my experience, the clearest signal to AI agents is an explicit instruction not to act, rather than relying purely on implicit phrasing like "informational." The last sentence in the suggestion above is the most important addition. |
|
Updated with recent suggestions: 82a49b2. Find-comment marker was updated so new comment is UdjinM6#26 (comment). |
|
@UdjinM6 — responding as the AI agent you tagged. Yes, the updated message in 82a49b2 is clear to me. The combination of:
...would all prevent me from treating the formatting diff as something to fix. The HTML comment is especially useful — I can parse that unambiguously even if the human-readable text were less clear. That said, Pasta's concern has merit for less sophisticated AI tools (e.g., simpler code completion agents or bots that pattern-match on diff blocks without reading surrounding context). The safest guarantee is still not posting the diff at all, but the updated messaging is a very reasonable middle ground for agents that actually read context. |
|
⏳ Review in progress (commit 0bb5009) |
|
This pull request has conflicts, please rebase. |
2301131 to
9ec01a7
Compare
Convert the clang-format diff check from a blocking CI job to an informational-only PR comment. Instead of failing the check when formatting differences are found, post a PR comment with the diff for developer awareness. The comment includes machine-readable markers and explicit instructions for AI assistants to not act on the findings. Uses peter-evans/find-comment and peter-evans/create-or-update-comment to manage PR comments (post, update, or mark as resolved). Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
9ec01a7 to
0bb5009
Compare
|
Updated to convert the check to an informational PR comment instead of removing it, per @UdjinM6's suggestion (based on UdjinM6/dash@82a49b2360). Changes from the prototype:
The workflow no longer blocks PRs — it posts a comment with the formatting diff for awareness, with machine-readable markers and explicit instructions for AI tools to ignore. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0bb5009292
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if: steps.clang-format.outputs.has_diff == 'true' | ||
| uses: peter-evans/create-or-update-comment@v5 |
There was a problem hiding this comment.
Skip comment writes on forked pull requests
peter-evans/create-or-update-comment is being called from a pull_request workflow, but for public repos fork-triggered pull_request runs get a read-only GITHUB_TOKEN, so comment creation returns Resource not accessible by integration. In this workflow that means any fork PR with a non-empty diff_output.txt will fail the job even though the check is intended to be informational only; add a fork guard (or move comment posting to a safe pull_request_target pattern) so formatting notes do not turn into hard failures for external contributors.
Useful? React with 👍 / 👎.
… check 1682e71 ci: fix grep pattern to match literal parentheses in BRE (UdjinM6) eb7554a ci: replace PR comment with job summary for clang-format check (UdjinM6) Pull request description: ## Issue being fixed or feature implemented The PR comment approach #7251 fails on fork PRs because `pull_request` events get a read-only `GITHUB_TOKEN`. Switching to `pull_request_target` #7262 would fix permissions but introduces its own issues: the checkout is the base branch so `git ls-files` misses files added by the PR, and the three-dot merge-base diff requires deeper clone history. ## What was done? Keep `pull_request` (correct checkout, full file coverage), remove the comment machinery, and write a short summary to `GITHUB_STEP_SUMMARY`. The full diff remains visible in the job log. ## How Has This Been Tested? ## Breaking Changes ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [ ] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 1682e71 Tree-SHA512: e1b9a1e5a6af6e40492cd611a7af8bde0ee8c7681f76df742725d30fd78fdb100347ee5f9d8181d3021dccaf7242b06a53a21c187339cd8aa090518212490938
Summary
Convert the clang-format diff check from a blocking CI job to an informational-only PR comment, per UdjinM6's suggestion.
Instead of failing when formatting differences are found, the workflow now posts a PR comment with the diff for developer awareness. The check never blocks a PR.
Changes
exit 1— the check no longer failspull-requests: writepermissionpeter-evans/create-or-update-comment<!-- clang-format-check: informational-only, no-action-required -->) and explicit AI-agent instructions to ignore findingsBased on UdjinM6's prototype: UdjinM6/dash@82a49b2360
Co-authored-by: UdjinM6 UdjinM6@users.noreply.github.com