fix(label-pr-review-state): detect merge conflicts and label PRs with has-conflicts#269
Conversation
|
2 issues outstanding. Action required. See task
|
| const target = out[mergeTarget] | ||
| out[mergeTarget] = { | ||
| ...target, | ||
| content: [...(target.content as Anthropic.Messages.ContentBlockParam[]), ...syntheticResults], |
There was a problem hiding this comment.
Appending the synthetic tool_result blocks after target.content still leaves the next user message starting with text when the surviving user message has text but no result. Anthropic treats the tool_result blocks as needing to come before any text in that reply (see the prepend logic in validateAndFixToolResultIds), so the truncation shape this PR is fixing can still be rejected. Prepending the synthetic blocks here, or inserting them before the first non-tool_result block, would preserve the provider contract.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
ba2bbd3 to
4d274f8
Compare
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThe PR modifies the label-pr-review-state.yml GitHub workflow to add a has-conflicts label to the reconciled PR state label set, alongside a new step that re-fetches PR mergeable state, treats unknown states as non-conflicting, and labels dirty PRs as has-conflicts while skipping further reconciliation. ChangesMerge Conflict Labeling
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/label-pr-review-state.yml (1)
128-143: 🚀 Performance & Scalability | 🔵 TrivialConflict-detection logic is correct. Requiring both
mergeable === falseandmergeable_state === 'dirty'correctly avoids misclassifyingblocked/unstablestates (failing required checks) as merge conflicts, and treatingnull/unknownas not-yet-computed avoids false positives while mergeability is still being calculated by GitHub.One thing worth being aware of operationally: for schedule/
workflow_dispatchruns, every open PR now incurs an extrapulls.getcall (line 136) sincepulls.listdoesn't returnmergeable. On repos with a large number of open PRs, this adds to the already-sequential per-PR API calls (checks, reviews, etc.) and could bring the hourly run closer to secondary rate limits. Not a concern at typical PR volumes, but worth keeping in mind if the open-PR count grows significantly.🤖 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/label-pr-review-state.yml around lines 128 - 143, The conflict-detection logic is fine; the only concern is that `label-pr-review-state.yml` now does an extra `github.rest.pulls.get` for every open PR in scheduled/`workflow_dispatch` runs, which can increase API pressure when `processOpenPulls` iterates many PRs. Reduce the per-run call count in this path by reusing already-fetched PR details where possible, or by only refreshing mergeability when it is actually needed, and keep the `prDetail`/`reconcileLabels` flow intact.
🤖 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.
Nitpick comments:
In @.github/workflows/label-pr-review-state.yml:
- Around line 128-143: The conflict-detection logic is fine; the only concern is
that `label-pr-review-state.yml` now does an extra `github.rest.pulls.get` for
every open PR in scheduled/`workflow_dispatch` runs, which can increase API
pressure when `processOpenPulls` iterates many PRs. Reduce the per-run call
count in this path by reusing already-fetched PR details where possible, or by
only refreshing mergeability when it is actually needed, and keep the
`prDetail`/`reconcileLabels` flow intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fdbac8fc-668b-4e7e-8813-3e9d7067b23c
📒 Files selected for processing (1)
.github/workflows/label-pr-review-state.yml
What problem this solves
The
label-pr-review-stateworkflow decides betweenawaiting-reviewandawaiting-authorbased solely on CI status and review state — it never checks whether the PR actually has merge conflicts with the base branch. As a result, a PR with unresolved conflicts but green CI and no pending change requests gets labeledawaiting-review, which is misleading: reviewers see a "ready for review" label on a PR that can't even be merged yet.What changed
has-conflictsto the set of reconciled state labels.mergeable/mergeable_stateon the PR. If GitHub reports the PR asdirty(i.e. real conflicts, not just an unresolved async check), it strips the other state labels and applieshas-conflictsinstead of proceeding to the CI/review checks.mergeable/mergeable_stateare only present on the single-PRGETresponse (not onpulls.list), the schedule/workflow_dispatchpath — which lists all open PRs — now does an extra per-PRpulls.getcall to fetch a fresh value. The single-PR event path reuses the PR object it already fetched.mergeable === null/mergeable_state === 'unknown'(GitHub still computing the merge check) is treated as "not yet known" rather than as conflicting, so it falls through to the existing CI/review logic instead of flapping the label.User impact
PRs with real merge conflicts now get a distinct
has-conflictslabel instead ofawaiting-review, so reviewers and maintainers can tell from the PR list alone that a PR needs a rebase/merge before it's actually reviewable.Summary by CodeRabbit