Force push does not invalidate existing approved reviews

[anonrig]

If the author force pushes to their branch after a reviewer approved it, I assumed that it would invalidate existing approved reviews, since the code that was reviewed is changed. But right now approved reviews count as valid ones, and might lead to unwanted code to be merged to main.

Example nodejs/node#46904

➜  node git:(main) ✗ git node metadata 46904
✔  Done loading data for nodejs/node/pull/46904
----------------------------------- PR info ------------------------------------
Title      url: use private properties for brand check (#46904)
Author     Yagiz Nizipli <[email protected]> (@anonrig)
Branch     anonrig:url-context -> nodejs:main
Labels     semver-major, performance, whatwg-url, esm, author ready, worker, needs-ci
Commits    1
 - url: use private properties for brand check
Committers 1
 - Yagiz Nizipli <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/46904
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
--------------------------------------------------------------------------------
   ℹ  This PR was created on Wed, 01 Mar 2023 21:28:43 GMT
   ✔  Approvals: 3
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/46904#pullrequestreview-1320667498
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/46904#pullrequestreview-1320668469
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/46904#pullrequestreview-1321281126
   ✖  This PR needs to wait 21 more hours to land
   ✖  GitHub CI is still running
   ℹ  Last Full PR CI on 2023-03-02T23:13:10Z: https://ci.nodejs.org/job/node-test-pull-request/50175/
✔  Build data downloaded
   ✖  Last Jenkins CI still running

[aduh95]

It’s always been the case, and I think it aligns with our rules defined in docs/contributing. The CQ will refuse to land a PR that had commits pushed to it since the last review though.

[anonrig]

If there is one review after the last force push, then CQ will merge it, right? I think it's still open to abuse (and a bug).

[aduh95]

If there is one review after the last force push, then CQ will merge it, right?

That's correct.

I think it's still open to abuse

What abuse do you have in mind?

(and a bug)

What do you think should be the correct behavior?

[anonrig]

If person X opens a pull-request and receive 2+ reviews (from Y and Z), he could force-push couple of hours before the 48 hour window. There is a pattern of Node.js collaborators of approving already approved pull-request without thorough reviews, and this pattern will lead to approving that force-pushed branch, which will merge by CQ.

I just summarized a method of pushing unwanted code to main, but in general, the code that Y signed, reviewed and approved does not exist anymore, but when CQ merges the pull-request it will add comment to the commit saying Approved-by: Y, and that person will be liable for this commit. In that scenario, is Y or Z really liable? If yes, why, if not, why are we including Y and Z's name on the git commit?

[anonrig]

@aduh95 For example: I force pushed, nobody reviewed it, and it still got merged by CQ. (Reference: nodejs/node#46904)

[aduh95]

@aduh95 For example: I force pushed, nobody reviewed it, and it still got merged by CQ. (Reference: nodejs/node#46904)

I think that’s definitely a bug. Note that Jacob did review it (but didn’t approve it), so maybe that explains why the CQ didn’t flag it.

[aduh95]

If person X opens a pull-request and receive 2+ reviews (from Y and Z), he could force-push couple of hours before the 48 hour window.

Why would force-pushing be different than pushing additional commits though? In both cases, the end result (the code actually being merged) can be very different from the one that was reviewed (the only difference I can see is that force pushing creates a much worse reviewing experience).

There is a pattern of Node.js collaborators of approving already approved pull-request without thorough reviews, and this pattern will lead to approving that force-pushed branch, which will merge by CQ.

That’s a different issue, folks should not do that (and I don’t know on what info you base this affirmation, I’m not sure it’s even true, but it’s not worth discussing anyway, as it’s besides your point)

I just summarized a method of pushing unwanted code to main, but in general, the code that Y signed, reviewed and approved does not exist anymore, but when CQ merges the pull-request it will add comment to the commit saying Approved-by: Y, and that person will be liable for this commit. In that scenario, is Y or Z really liable? If yes, why, if not, why are we including Y and Z's name on the git commit?

Yes and no, we expect the author to re-request reviews when the code changed significantly since the review. In most cases though, the code does not change significantly, so it actually makes more sense to keep the complete list of the reviewers.
Being liable is not a very well defined thing in this scenario anyway, and problems around that so rarely occur that I think it’s safe to say our current system works pretty well.

[targos]

There's apparently a little bug to fix, but I think we should also not aim to rely only on automation and keep trust in collaborators. It's rarely a big issue if something bad lands on main anyway. If there is a pattern of people trying to cheat the system (to be proven), we should fix that (with education/documentation).
By the way, and TBH (my intention is not to blame you), I think it was a mistake to add the commit-queue label to nodejs/node#46904 if you thought it still needed reviews.

[aduh95]

Should we close this as Won't fix? It seems most folks are happy to be listed as an approver wether or not there was a force-push, and as said above regular pushes can also change drastically what the PR does.

Now that #680 has been released, the CQ should never land a PR if there wasn't at least one approving review from someone with commit access (which was an issue reported in #677 (comment)), and the rest of this ticket seems like a non-issue to me.

[RafaelGSS]

It seems most folks are happy to be listed as an approver wether or not there was a force-push, and as said above regular pushes can also change drastically what the PR does.

This might be the case for patch/semver-minor PRs, but I can argue that might be bad for semver-major PRs.

[thisalihassan]

I believe this is a good idea I accidently caused a bug in this PR (but later I fixed it with another PR) it wasn't caught because my PR was already approved by everyone