ci: tolerate conflict comment permission errors#7322
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
0849723 to
6948ad3
Compare
|
✅ Review complete (commit 6948ad3) |
|
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)
WalkthroughThe PR updates the GitHub Actions workflow file to upgrade the Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Small, well-scoped CI fix that upgrades the sticky-comment action to mshick/add-pr-comment@v3 and marks comment publication/removal as continue-on-error, while preserving the final 'Fail if conflicts exist' step as the authoritative gate. Verified at 6948ad3: only .github/workflows/predict-conflicts.yml is touched, the conflict enforcement step (line 60-62) remains unchanged, and continue-on-error correctly scopes only to comment-posting failures. No blocking issues; the two nitpicks are pre-existing observations unrelated to this PR's scope.
Reviewed commit: 6948ad3
💬 2 nitpick(s)
1 additional finding
💬 nitpick: 'Remove conflict comment' step overwrites rather than removes
.github/workflows/predict-conflicts.yml (lines 50-59)
Pre-existing behavior, untouched by this PR: the if: ... == 'false' branch calls add-pr-comment with a 'No Merge Conflicts Detected' body rather than removing the prior sticky. Because both steps share message-id: conflict-prediction, the action overwrites the existing sticky comment, so the net visible effect is acceptable. The step name slightly mismatches its behavior. Not in scope; calling out only because the PR description mentions 'removal' semantics.
| if: steps.validate_conflicts.outputs.has_conflicts == 'true' | ||
| uses: mshick/add-pr-comment@v2 | ||
| continue-on-error: true | ||
| uses: mshick/add-pr-comment@v3 |
There was a problem hiding this comment.
💬 Nitpick: Floating @V3 tag for third-party action
Both updated steps reference mshick/add-pr-comment@v3 by floating major-version tag rather than a pinned commit SHA. This matches existing convention in this workflow (actions/checkout@v4, PastaPastaPasta/potential-conflicts-checker-action@v0.1.10), so it is not a regression. Worth noting that the workflow runs with pull-requests: write and is triggered by pull_request_target / pull_request_review, so a compromised tag could in principle act on PRs. Out of scope for this PR; pinning policy would be a cross-workflow change.
source: ['claude']
|
/rerun |
PR: tolerate conflict comment permission errors
Summary
mshick/add-pr-comment@v3Fail if conflicts existgate authoritativeCloses #7268.
Motivation
Fork/review-triggered
predict_conflictsruns can fail withResource not accessible by integrationwhile trying to update PR comments,even when conflict detection itself has already completed. Comment delivery
should be best-effort; actual conflicts should continue to be enforced by the
final gate step.
Validation
.github/workflows/predict-conflicts.ymlwithyaml.safe_load.git diff --check upstream/master...HEAD.code-review dashpay/dash upstream/master ci-predict-conflicts-comment-resilience.Result:
Recommendation: ship.