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.
There was a problem hiding this comment.
it almost sounds like @psschwei wants to disable auto-merge, but I know it ain't so
I go back and forth... I think, this morning, I leans towards reviewer merges (which makes the auto-merge question moo), but I reserve the right to wake up smarter tomorrow...
There was a problem hiding this comment.
I personally like the PR creator to merge; if they want the reviewer to merge, they can just set auto-merge.
I also think request changes should be soft blocking; ie the pr opener can dismiss a review and move on without it but that should be an operation imbued with seriousness and responsibility.
| | `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 do think we should at least make it socially acceptable to say that you would prefer someone else to review this area of the code. For instance, if I was requested on a PR about telemetry not opened by Alex, I would ask that he be requested as well (or even review instead of me).
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?) |
|
We should also probably have a stance on resolving conversations in PRs (?) |
Valid - I usually automatically resolve things when I push an updated and they become outdated (or are outdated even if the github UI doesn't think so) for simple do foo type things. For more complex or open ended things that were maybe a question instead of a directive anyway I usually leave those open 🤔 |
Fold in discussion from the PR review-states doc into a fuller policy: add a Review Assignment section (CODEOWNERS subset, one-approval rule, deferring to area owners), a Merging section taking the maintainer-action hybrid position and documenting the do-not-merge/hold label, and a Review comments section covering suggestion blocks and thread resolution. Tighten the APPROVE and REQUEST CHANGES bullets so nits require no follow-up and holding for re-review requires a concrete concern. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
Pushed an update folding in the feedback so far. Summary of what changed and what's still open. Incorporated (where we'd converged):
Deliberately took a position — please push back if you disagree:
Left out, want input before encoding:
|
I'm happy to go with whatever the majority decides, especially as our process here is still a bit manual. If we were using more automation / gitops (e.g. prow), I would have a stronger opinion.
I'm not sure what exactly the change was here, but nothing looks amiss.
In the past, I've used this in two ways: the non-actional items like you say, but also as a chance for lazy consensus without blocking (i.e. I approve but hold, which allows others to take a look. If no one does within a decent timeframe, can remove the label and merge) The latter instance matters more if we have automation that merges right after approval. |
planetf1
left a comment
planetf1
left a comment
There was a problem hiding this comment.
Three suggestions inline — no blocking issues.
| **`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 needs to validate a concrete concern (a suspected regression, an incomplete change) before the PR merges, and is withholding approval until the next round. Wanting another pass without a specific concern isn't on its own a reason to block; coordinate with the author by comment instead. | ||
| - 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.
The intent here is clear, but a reader encountering this cold might wonder: once the issues are filed, who re-approves and when does the block clear? Suggesting an exit condition to close the loop:
| - 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. | |
| - Important follow-up issues have not yet been opened, and the reviewer doesn't want them forgotten. The expectation is that the issues get filed — not that the PR itself changes — at which point the reviewer re-approves. |
|
|
||
| ### Merging | ||
|
|
||
| Merging is a maintainer action, performed by either the author or a reviewer; "merge when ready" only schedules it once requirements pass. Whoever merges confirms the PR is actually ready, not just that the queue requirements are green. CI passing and the minimum approval make a PR *mergeable*, but a concern raised in discussion that never became an explicit `REQUEST CHANGES` should still be resolved first. |
There was a problem hiding this comment.
"Whoever merges confirms the PR is actually ready" is sound guidance, but there's no stated mechanism for the merger to discover unresolved concerns. One option is to make this self-enforcing: GitHub's branch-protection setting "Require conversation resolution before merging" would enforce it at the repo level and remove the need for the prose instruction entirely. Worth considering alongside this doc update.
cc @psschwei — you were discussing the auto-merge / merge-gate setup earlier in this thread, so this seems relevant.
There was a problem hiding this comment.
I like that
edit: and by that, I mean requiring conversation resolution
|
|
||
| Merging is a maintainer action, performed by either the author or a reviewer; "merge when ready" only schedules it once requirements pass. Whoever merges confirms the PR is actually ready, not just that the queue requirements are green. CI passing and the minimum approval make a PR *mergeable*, but a concern raised in discussion that never became an explicit `REQUEST CHANGES` should still be resolved first. | ||
|
|
||
| The `do-not-merge/hold` label blocks merging while it is applied, enforced by CI independent of review state. It is a separate mechanism from a `REQUEST CHANGES` review and is removed once the blocking condition clears. |
There was a problem hiding this comment.
| The `do-not-merge/hold` label blocks merging while it is applied, enforced by CI independent of review state. It is a separate mechanism from a `REQUEST CHANGES` review and is removed once the blocking condition clears. | |
| The `do-not-merge/hold` label blocks merging while it is applied, enforced by CI independent of review state. It is a separate mechanism from a `REQUEST CHANGES` review and is removed by a maintainer once the blocking condition clears. |
There was a problem hiding this comment.
I'd rather not specific this here as anyone with triage access could edit labels, not just maintainers.
Spell out how the block clears: filing the issues, not changing the PR, after which the reviewer re-approves. Assisted-by: Claude Code Signed-off-by: Alex Bozarth <ajbozart@us.ibm.com>
|
A codex generated summary of the doc: Defined scenarios
Scenarios not definedThe current proposal does not establish rules for:
Overall multi-reviewer policyThe proposal's effective multi-reviewer policy is:
The more difficult coordination case—several reviewers independently requesting |
|
That summary is pretty solid, my opinions on a couple of the "Scenarios not defined" items:
Unless there is a reason for a maintainer to dismiss a review only the reviewer should do so, I can specifiy this if necessary, but this is standard GitHub PR practice so I didn't think It was worth specifying
I wanted to add this, but I couldn't come up with a list of good reasons without feeling like I was limiting its use by documenting them |
It may be that there isn't a good reason and we should get rid of it (wouldn't hurt my feelings) |
Oh I think there are good reasons to keep it. I just think that documenting the reasons I can think of myself would limit its use in the minds of those that read the doc. (ie if I doc its used for blocking due to upstream work or design decisions someone might read that and not use it for lets say "don't merge yet it will break another in flight PR" or "don't merge until after the release" etc) |
398455d
7 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.