Minimum waiting time for PRs with recently pushed commits

[LiviaMedeiros]

Problem

There are guidelines regarding waiting time in PRs:

node/doc/contributing/pull-requests.md

Lines 421 to 432 in c1ccade

	### Respect the minimum wait time for comments
	There is a minimum waiting time which we try to respect for non-trivial
	changes, so that people who may have important input in such a distributed
	project are able to respond.
	For non-trivial changes, pull requests must be left open for at least 48 hours.
	Sometimes changes take far longer to review, or need more specialized review
	from subject-matter experts. When in doubt, do not rush.
	Trivial changes, typically limited to small formatting changes or fixes to
	documentation, may be landed within the minimum 48 hour window.

node/doc/contributing/collaborator-guide.md

Lines 179 to 181 in c1ccade

	### Waiting for approvals
	Before landing pull requests, allow 48 hours for input from other collaborators.

node/doc/contributing/pull-requests.md

Lines 344 to 352 in c1ccade

	### Step 10: Landing
	In order to land, a pull request needs to be reviewed and [approved][] by
	at least two Node.js Collaborators (one collaborator approval is enough if the
	pull request has been open for more than 7 days) and pass a
	[CI (Continuous Integration) test run][]. After that, as long as there are no
	objections from other contributors, the pull request can be merged. If you find
	your pull request waiting longer than you expect, see the
	[notes about the waiting time](#waiting-until-the-pull-request-gets-landed).

However, the time is always calculated since PR creation and not since last pushed commits (even for significant changes and/or force-pushes).
This effectively creates a situation where a single collaborator approval (and a single green CI for needs-ci PRs that need a full CI run. PRs) is enough to immediately merge PR right after push - both from guidelines's and NCU/ commit-queue Add this label to land a pull request using GitHub Actions. 's points of view - as long as it was initially opened 7+ days ago.

Somewhat related: nodejs/node-core-utils#677

Possible solutions

• Make it strict, always counting the 48/168 hours from last push rather than initial open date.
• Make it semi-strict, e.g. counting ~24 hours from last push if PR passes open date limit.
• Make doc-only addition to guideline, asking to wait ~24 hours if anything non-trivial was pushed.
• Keep it as is and rely on common sense.

Doc-only option looks best to me since it won't slow down landing trivial fixups, but it's also always possible to fast-track them or land manually. Maybe there are different opinions on this.

Just to clarify, this issue is not a security concern nor a loophole for potential bad actors etc. The minimum waiting time is supposed to allow anyone to provide their input (including suggestions and explicit blocks), and not having it for subsequent commits sometimes nullifies this ability.

[panva]

Keep it as is and rely on common sense.

👍

[joyeecheung]

I would say it's good enough to just keep it as is. If the new change is substantial enough we should trust people to ask for re-review and give it enough time. More often than not I see people saying things like "if no objections by the end of the week I will land it" instead going straight ahead landing it after changing things substantially.

On a side note, one thing I like about Chromium/V8 is that they even allow small unreviewed changes to be committed after a CL is approved so that people don't have to wait for another approval after fixing up nits from "LGTM with nits" (I am not sure how the automation decides it's okay to not require another review, maybe just by the size of the new diff). And these are projects owned by a company where many code has to be approved by their employees and are released very quickly & automatically to canary channels. I'd say for Node.js which has open governance and more trust in collaborators and a staged release process, it's weird that we have less trust in people.

If I were to propose a change I would propose something like this to make the process even less strict. If it's just some additional nits like fixing a typo or making linter happy, there is no need for re-approval if the change is made by a collaborator and maybe collaborators can just apply a label to indicate the additional changes & to make the commit queue happy with it. As usual they should use good judgment as to when the change is substantial enough to require an approval, of course.

[aduh95]

This effectively creates a situation where a single collaborator approval (and a single green CI for needs-ci PRs) is enough to immediately merge PR right after push - both from guidelines's and NCU/ commit-queue 's points of view - as long as it was initially opened 7+ days ago.

It's not exactly correct, the CQ will refuse to land a PR if there has been commits pushed since the last review – in which case the PR must either wait for another collaborator approval or be landed manually.

[mhdawson]

I'm +1 for leaving as is, as I don't think we've seen a problem with the way it is.

[LiviaMedeiros]

I would say it's good enough to just keep it as is. If the new change is substantial enough we should trust people to ask for re-review and give it enough time.

Agreed (hence why I also would prefer doc-only change). Again, it's not about any trust problems, but about current guidelines and NCU behaviour. "There's a minimum waiting time since last push that can be ignored if changes were insignificant" would make more sense than "There's a minimum waiting time since the moment PR was originally opened, and also consider waiting after subsequent pushes if changes were not trivial", IMHO.

If I were to propose a change I would propose something like this to make the process even less strict.

This is not mutually exclusive. I agree that collaborators should be able to override the automated decision and land formally-not-ready PRs if there's a strong reason to do so, right now it's impossible with commit-queue but easy to do with git node land. If we do doc-only change, the process won't become any different; and if the strict time frame gets implemented, we can use fast-track or consider adding a convenient mechanism to land PR with loosened checks. For example: collaborator places force-commit-queue label, bot makes a post similar to how fast-track works, and lands the PR after 24 hours if there was no objection and PR fulfills loosened checks (e.g.: at least 1 approve since last change, at least one green CI even if it ran on previous commit, green GH CI).

Also, if there will be change to NCU, it wouldn't necessarily make landing PR harder, since it would simply delay commit-queue's action. It would indeed slow down the process though.

Automatic detection if latest changes were nits or not would be great, however I don't think such detection can be implemented in robust way, except for purely cosmetic changes (commas, whitespace, comments, etc.). Diff size doesn't sound like a reliable metric for that.

It's not exactly correct, the CQ will refuse to land a PR if there has been commits pushed since the last review – in which case the PR must either wait for another collaborator approval or be landed manually.

I assume that PR author isn't necessarily a collaborator, so 'single approval' meant exactly this.
For example: author A makes initial version of PR, collaborators B C D give LGTM approval, 7 days passed. Afterwards, collaborator E suggests changes (explicit red change request, or 'LGTM with or without nits' approval, doesn't matter). Shortly after that, A applies suggestions, E reapproves and lands the PR (manually or using commit-queue, doesn't matter) with four Reviewed-by.
In this situation, E can become not only the only reviewer of final version of what gets landed, but also co-author who approved their own suggestion. If it happens fast enough (especially if PR doesn't need CI run), even B C D may have no time to review final changes despite being notified by GH.

Even though normally it doesn't cause any issues (and even if it did, it could be fixed in follow-up), it's strange that we rely only on E's judgement that additional changes are insignificant/final any ready to land, while for newly opened PRs NCU and guidelines define strict minimum waiting time.

---

To rephrase it differently, it can be seen from 'responsibility' angle rather than 'trust'. Just like how approving a pull request indicates that the collaborator accepts responsibility for the change., fast-tracking comes with responsibilities of decision that changes are eligible for landing earlier than in 48 hours, and landing comes with responsibility of decision that PR is ready to land without waiting for potential objections/suggestions.
Declaring minimum waiting time would simply make this responsibility more explicit.

[aduh95]

For PRs opened more than 7 days ago, a single approval is enough, so IIUC the problem you're trying to address is not that E is the sole reviewer of the final version, but that B, C, and D are listed as reviewers in the commit metadata, correct? I personally don't have a problem with that, it's quite rare that the PR content changes significantly after I reviewed it, and I'm fine trusting the judgement of my fellow collaborator doing the final approval.
If someone approves a PR, but disagree with a change that was made after their approval, I think the commit would get quickly reverted and the PR reopened until consensus is reached. It's not a formal process atm, and it probably doesn't need to be given things are working well as is IMO.

My understanding is that the minimal wait time is there so looking at the repo once per day is more than enough to see all the proposed changes and subscribe/chime in to the ones of interest. Subsequent commits should not affect this – but again, it's a judgement call that needs to be made, and I don't think we can automate a judgement call.

hence why I also would prefer doc-only change

I don't think "doc-only" would be a good metric either, a doc-only change can be significant

Declaring minimum waiting time would simply make this responsibility more explicit.

It would also be quite annoying tbh, one could argue that PRs are currently landing too slow, not too fast.

[jasnell]

+1 for keeping things as they are

[LiviaMedeiros]

For PRs opened more than 7 days ago, a single approval is enough, so IIUC the problem you're trying to address is not that E is the sole reviewer of the final version, but that B, C, and D are listed as reviewers in the commit metadata, correct?

No, the issue is that E is the sole reviewer. The strict 'PR was opened more than 7 days ago' rule doesn't have meaningful effect here: it just gets automatically fulfilled even if PR was opened as draft for a week, and even if 'last minute changes' were significant.

The metadata was mentioned only as part of how it works for the B C D there. Neither GitHub UI nor NCU invalidates existing approvals after pushing new commits, which makes collaborators not forced to re-approve PR each time, which de facto makes not-changing-approval-status equivalent of approval for subsequent commits.¹ This makes sense when previous reviewers have time to see new changes, but looks wrong if they haven't.

My understanding is that the minimal wait time is there so looking at the repo once per day is more than enough to see all the proposed changes and subscribe/chime in to the ones of interest. Subsequent commits should not affect this – but again, it's a judgement call that needs to be made, and I don't think we can automate a judgement call.

Yes, exactly. This is why I was suggesting 24 hours, plus it accounts timezone differences. Worth mentioning that waiting for potential objections isn't limited to collaborators, anyone can track PR and provide their input.
Although we can't automate a judgement call, we can formalize the recommended decision making process in the collaborator guide as something like If PR has commits pushed less than 24 hours ago, please consider waiting before landing, unless you are fully confident that these commits are minor and will not raise any objections. or Avoid landing PRs with commits pushed in the last 24 hours unless you're certain they are minor and uncontroversial.

I don't think "doc-only" would be a good metric either, a doc-only change can be significant

Doc-only in this context means that it won't directly disrupt or slow down anyone's workflow, i.e. both commit-queue and git node land would work same as before.

Footnotes

1. Assuming there is E's approval that fulfills the 7 days rule as well as request-ci handler. ↩