docs: document PR review states#1306
Conversation
Adds a Review States subsection under the Pull Request Process in CONTRIBUTING.md describing when to use APPROVE, REQUEST CHANGES, or COMMENT, plus a one-line pointer in AGENTS.md so AI reviewers find it. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
github-actions
Bot
added
the
documentation
|
Here's a first stab at a short doc update with how to choose a PR review state as we discussed in scrum. This was generated based on the notes I transcribed from memory, so it could easily have mistakes compared to our discussion points |
| **`REQUEST CHANGES`** — the PR should not merge in its current form. Use when: | ||
| - There is an actual blocking or breaking issue (correctness, regression, missing tests, etc.). | ||
| - The reviewer wants to personally validate something before the PR merges, and is withholding their approval until the next round. | ||
| - Important follow-up issues have not yet been opened, and the reviewer doesn't want them forgotten. The expectation here is that the issues get filed, not that the PR itself changes. |
There was a problem hiding this comment.
I could see another scenario here: I want to be able to re-review before this merges. That is similar to your second bullet, but the intent is slightly different.
There was a problem hiding this comment.
I wonder if we should just soften bullet two to that. I could see it as a subset of that scenario
There was a problem hiding this comment.
I'm not sure 'I want' is enough in it's own right. it should be supported by the other criteria - ie believing there'd be a regression, or that the code is incomplete/slightly regressive without followups.
There was a problem hiding this comment.
It's not always quite so cut and dry. Sometimes reviews are a conversation (why did you do this approach? Have you thought about that other approach? etc.), and in those cases the original reviewer might want to be able to re-review. This also ties in with the auto-merge discussion below. I guess the larger question (which we're sort of hinting around without explicitly addressing) is, when is it ok to merge a PR?
There was a problem hiding this comment.
if you don't "request changes"/block then you have to realize that a merge might happen and comments/threads may get lost (missed/forgotten). But we also have a typically better behavior than that, and the norm is that nobody presses the merge button w/o proper follow-up. It's not perfect. When stuff gets missed this way the fix is to enter issues, have discussions... If that isn't good enough for a concern then it should be a request changes.
There was a problem hiding this comment.
Agreed with actual blocking portion or important followup not open yet. I think the "I want to re-review" portion makes sense as well, though in this case I think having the blocking change request is a prerequisite.
As for Paul's comment about when it's OK to merge my 2C:
- All of CI passes
- Minimum approvals are already in, ie one could actually merge
- Nobody has requested a major change that is not yet addressed - this is a small caveat as sometimes there is a discussion here without an explicit requests changes.
|
|
||
| ### Review States | ||
|
|
||
| When submitting a PR review, pick one of GitHub's three states. Using them consistently keeps the merge signal meaningful: an `APPROVE` should mean the reviewer is willing to support the change, and a `REQUEST CHANGES` should mean something is actually blocking. |
There was a problem hiding this comment.
I'm not sure I agree request changes == blocking. I think it just means I'm requesting changes before I approve.
There was a problem hiding this comment.
Another thing to note: if a PR is set to auto-merge, an approve does not allow others a chance to weigh in (though it also means the original reviewer check back later if they don't approve and no one else does).
We also have a do-not-merge/hold label that in theory can be used with an approve to work around that... not elegant but I think this is a scenario we may need to account for.
There was a problem hiding this comment.
I'm not sure I agree request changes == blocking. I think it just means I'm requesting changes before I approve.
I agree that the wording could be better, but from a technical perspective it is true, our CI does block merging when requests changes is true.
if a PR is set to auto-merge, an approve does not allow others a chance to weigh in
This only happens if a requests change review doesn't also exist, which lines up with the I want to be able to re-review before this merges idea above, but a reviewer could still miss if they haven't submitted a review yet (like if they're mid review locally)
We also have a do-not-merge/hold label
I could see including that as part of this doc, but I'm not sure in what way would be best
There was a problem hiding this comment.
if a PR is set to auto-merge, an approve does not allow others a chance to weigh in
This only happens if a requests change review doesn't also exist, which lines up with the I want to be able to re-review before this merges idea above, but a reviewer could still miss if they haven't submitted a review yet (like if they're mid review locally)
One of the the scenarios I'm thinking of is something like this:
- You and I are tagged as reviewers on a PR
- You review, and the author makes changes to address your review
- I review, and the author makes changes to address my review
Who approves in that scenario? In theory we both should, but if auto-merge is on only one of us can. Which is mildly annoying. Not majorly, because there's numerous ways to work around this (requesting changes, 1st reviewer just comments, approve+hold, get rid of auto merge, only have one person review a PR, etc.). Just nothing that feels exactly right (though in the real world, sometimes good enough is good enough 😄 )
There was a problem hiding this comment.
I agree that the wording could be better, but from a technical perspective it is true, our CI does block merging when requests changes is true.
Fair enough. The bullets below are more nuanced as well, so all good.
There was a problem hiding this comment.
it almost sounds like @psschwei wants to disable auto-merge, but I know it ain't so
There was a problem hiding this comment.
I can see the issue here with multiple requested changes from multiple people - at the end of the day I think the LGTM/approve bestows with the co-responsibility of understanding/maintaining that area in the future.
We could have a community guideline that if there are multiple changes requested reviews, all but the last would simply "dismiss" their review - which becomes unblocking. This guideline would mean the last person with changes requested not yet re-reviewed would be the only approval.
| | `COMMENT` | Anything else, or a follow-up that doesn't change the prior status. | | ||
|
|
||
| **`APPROVE`** — the reviewer is willing to support the change. | ||
| - The PR is ready to merge as-is, with or without non-blocking nits or follow-up suggestions that the author may address or skip with no harm. |
There was a problem hiding this comment.
and specifically that don't require any followup
|
|
||
| **`REQUEST CHANGES`** — the PR should not merge in its current form. Use when: | ||
| - There is an actual blocking or breaking issue (correctness, regression, missing tests, etc.). | ||
| - The reviewer wants to personally validate something before the PR merges, and is withholding their approval until the next round. |
There was a problem hiding this comment.
I would say that they have a significant concern they need to validate.
| **`REQUEST CHANGES`** — the PR should not merge in its current form. Use when: | ||
| - There is an actual blocking or breaking issue (correctness, regression, missing tests, etc.). | ||
| - The reviewer wants to personally validate something before the PR merges, and is withholding their approval until the next round. | ||
| - Important follow-up issues have not yet been opened, and the reviewer doesn't want them forgotten. The expectation here is that the issues get filed, not that the PR itself changes. |
There was a problem hiding this comment.
I'm not sure 'I want' is enough in it's own right. it should be supported by the other criteria - ie believing there'd be a regression, or that the code is incomplete/slightly regressive without followups.
AngeloDanducci
left a comment
AngeloDanducci
left a comment
There was a problem hiding this comment.
May be worth adding to the document somewhere to encourage suggestion blocks for small nits for comment/approval scenarios.
Making it easy to incorporate non blocking but nice to have things from reviews could help reduce some of the review cycle friction.
|
One other thing to align on. We assign multiple reviewers. If a reviewer has already approved or requested changes should additional reviewers still make an effort to review in addition? Generally if I see a LGTM or a requested change I assume the responsibility on followups is already assumed and won't devote time to additional review - however more eyes can't hurt. |
As a reviewer if the content of the PR matter to me I'll sometime want to do my own review even if someone has already approved it, also if the only approval has no context, ie just a flat approval with not feedback at all I'll sometime also feel like I need to double check. Either way I usually ping the author on Slack to warn them I'm doing a review and the hold off on merge because the only other idea is to put a action less needs review just to block on yourself, which feels icky |
|
Another idea: we could disable auto merge and make it the responsibility of the reviewer(s) to merge. Almost every other project I've ever worked on has followed that model (and to be honest, I'm not sure why we aren't), plus it standardizes the process for both maintainer PRs and external contributor PRs. |
My experience has kind of been the opposite, PRs by maintainers have been merged by their authors (or at least either author or reviewer) on most of my OSS projects. Pre-AI I would probably be fine doing reviewer-merge, but given the prevalence of AI generated PRs and AI reviewers I tend to trust the author more than the reviewer nowadays to be the final controller. |
|
It may be good to add a "PR review process" section. It may explain:
|
I do not follow your logic here. |
I don't trust that if we document that it's up to reviewers to merge that an AI reviewer wouldn't try to merge things on it's own when posting approving reviews |
You can prevent that by using an appropriately-scoped PAT, having read-only permissions on Contents |
markstur
left a comment
markstur
left a comment
There was a problem hiding this comment.
Note: I wrote this earlier today and somehow it got stuck in pending, but it's fine...
LGTM -- so I'm going to COMMENT this APPROVAL (goes back to read again). :)
Yes, I have no problem with what's added. Thanks!
Sure, additional reviews and discussion here is expected.
|
|
||
| ### Review States | ||
|
|
||
| When submitting a PR review, pick one of GitHub's three states. Using them consistently keeps the merge signal meaningful: an `APPROVE` should mean the reviewer is willing to support the change, and a `REQUEST CHANGES` should mean something is actually blocking. |
There was a problem hiding this comment.
it almost sounds like @psschwei wants to disable auto-merge, but I know it ain't so
| **`REQUEST CHANGES`** — the PR should not merge in its current form. Use when: | ||
| - There is an actual blocking or breaking issue (correctness, regression, missing tests, etc.). | ||
| - The reviewer wants to personally validate something before the PR merges, and is withholding their approval until the next round. | ||
| - Important follow-up issues have not yet been opened, and the reviewer doesn't want them forgotten. The expectation here is that the issues get filed, not that the PR itself changes. |
There was a problem hiding this comment.
if you don't "request changes"/block then you have to realize that a merge might happen and comments/threads may get lost (missed/forgotten). But we also have a typically better behavior than that, and the norm is that nobody presses the merge button w/o proper follow-up. It's not perfect. When stuff gets missed this way the fix is to enter issues, have discussions... If that isn't good enough for a concern then it should be a request changes.
| **`COMMENT`** — for everything else. Use when: | ||
| - Posting a follow-up review that shouldn't change the PR's status from a prior `APPROVE` or `REQUEST CHANGES` (e.g., re-reviewing after a push when nothing material changed). | ||
| - The review falls between `APPROVE` and `REQUEST CHANGES` and the reviewer doesn't want to block. | ||
| - The reviewer wants to defer the final call to other reviewers. |
There was a problem hiding this comment.
Maybe somewhere we should mention something about deferring to other reviewers. Is there a concise rule that says you should comment about your intentions or just leave that to implied/common sense? Curious but wouldn't want to get to wordy about that here.
There was a problem hiding this comment.
I don't want this to get too big because people don't read anymore, but contrary to that, it's nice to have stuff written down when it helps.
A few things come time mind that I didn't see addressed (maybe off topic):
- we don't have an official stance on auto-merge -- ok... that means it is allowed
- we don't have a stance on "dismiss review". Common sense?
- I'm really curious about use of the resolve button. Sometimes I really like to use that as a contributor to clean up conversation, but the better practice is probably for the reviewer to do that. So stuff stays unresolved.
I don't think any of those ^^^ need to be part of this unless some interesting team decision follows.
I am used to the approver pushing the merge button, but somehow in this project it feels okay to have the committer do it. Maybe that's because a committer with multiple PRs might like to choose the order? Or maybe to let simmer in case another reviewer is still looking... As an approver, occasionally I push the button just to get things moving along (you're welcome?) |
6 participants
Pull Request
Issue
Fixes #
Description
Adds a
Review Statessubsection to CONTRIBUTING.md describing when to use GitHub's three PR review states —APPROVE,REQUEST CHANGES, andCOMMENT. Adds a one-line pointer in AGENTS.md Section 6 so AI reviewers find it.A few spots worth weighing in on during review:
REQUEST CHANGES— does the team agree this belongs there, or is it aCOMMENT+ re-request review?Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.