added script to require a review post push

[Bullrich]

Closes https://github.com/paritytech/opstooling/issues/174

Added a new step in the action that triggers review bot to stop approval from new pushes.

This step works in the following way:

• If the author of the PR, who is not a member of the org, pushed a new commit then:
• Review-Trigger requests new reviews from the reviewers and fails.

It does not dismiss reviews. It simply request them again, but they will still be available.

This way, if the author changed something in the code, they will still need to have this latest change approved to stop them from uploading malicious code.

Find the requested issue linked to this PR (it is from a private repo so I can't link it here)

[bkchr]

This way, if the author changed something in the code, they will still need to have this latest change approved to stop them from uploading malicious code.

So everyone will need to re-approve or how does this work? Aren't you already kicked from the merge queue when you add changes after it was activated?

[alvicsam]

Aren't you already kicked from the merge queue when you add changes after it was activated?

Yes and no. When a PR is in the merge queue you can't push changes: git will exit with an error. So you have to remove your PR from the queue to add the changes.

[Bullrich]

So everyone will need to re-approve or how does this work?

No, it only requests a review again, but it does not dismiss reviews. So, it blocks the system until a pair of eyes (old or new) reviews again.

Aren't you already kicked from the merge queue when you add changes after it was activated?

This works before getting to the merge queue.

If I make a commit modifying files in a sensitive directory (let's say CI) and I got the core-devs and release-engineers to approve my PR, I can still push a new commit adding some dangerous code to that area (specially an external individual) before adding it to the merge queue. This allows me to add a simple change and then add a destructive one after having your approval.

This change will make the review-trigger action not run when an external contributor pushes a new commit, and only when someone reviews the PR, it will stop the case of harmful commits being merged at last moment.

For example, when a regular member has a PR:

flowchart
    subgraph InternalPR
        ReviewBot[/Triggers Review Bot/]
        Member(Paritytech Member) --Creates PR-->ReviewBot
        Member(Paritytech Member) --Pushes new commit-->ReviewBot
        Reviewer(Reviewer) --Reviews PR-->ReviewBot
    end

Loading

But, when an external contributor has a PR, we have a slightly different flow:

flowchart
    subgraph ExternalPR
        ReviewBotForExternal[/Triggers Review Bot/]
        User(External contributor) --Creates PR-->ReviewBotForExternal
        User(External contributor) --Pushes new commit-->request[Request reviewers]
        InternalReviewer(Reviewer) --Reviews PR-->ReviewBotForExternal
    end

Loading

This way, even if a PR has 10 approvals, a new commit will require a member to re-review the PR. We do not discard the reviews, but require a review (or re-review) on this latest commit.

[bkchr]

before adding it to the merge queue

Can you add it to the merge queue as an external?

[Bullrich]

before adding it to the merge queue

Can you add it to the merge queue as an external?

@alvicsam can correct me if I'm wrong, but I believe you can not.

The objective of this task is to stop a hidden commit between obtaining all the approvals and enabling merge queue.

While a developer should always re-review everything before enabling a merge, it can be tricky to detect that.

In the linked ticket there is a detail explaining how a malicious actor could jeopardize the repository by @NachoPal.

[alvicsam]

@Bullrich that's right. Only users with write access can do that.

[bkchr]

Yeah, as I assumed. So, in general what you are changing here is to require someone to press approve again. However, I don't see how this changes anything? I mean your assumption is that "pressing approve" is requiring more thinking of one pressing approve?

Honestly, you want better accountability of people and not this change here. As long as this doesn't removes any approvals or whatever, I will not block it. BUT, it should be crystal clear what someone will need to do, because otherwise people will be confused why they can not merge.

[Bullrich]

I mean your assumption is that "pressing approve" is requiring more thinking of one pressing approve?

I'm also not particularly fond of the overhead it brings. It's gonna slow down the process of reviews.

The idea is that it would force users to look one more time before they press the button, but you are right on the concept that it can happen that people will still not look. But we also have a bigger problem that it takes only one person to err one time to jeopardize our whole CI infrastructure. This extra overhead could provide a bit more of accountability to the last reviewer.

It's also gonna have the same problem in runtimes once polkadot-fellows/runtimes#60 get resolved.

By the way, I'm planning on adapting polkadot-fellows/runtimes#76 to use this same logic.

---

Ideally, we could simply implement the following future:

But review bot doesn't get along with that feature as it dismisses the reviews from the users.

Honestly, you want better accountability of people and not this change here.

Completely agree with this thought! I would love to be able to provide more support in this area.

I'm currently working on some tools that could help reviewers in the future.

[mordamax]

@Bullrich @bkchr are we doing to proceed with this pr or there are still open quesitons?

[bkchr]

As long as this doesn't removes any approvals or whatever, I will not block it. BUT, it should be crystal clear what someone will need to do, because otherwise people will be confused why they can not merge.

Still this. Maybe let the bot post a comment telling what to do is the minimum.

[Bullrich]

@bkchr I have slightly modified the action.

Now, everytime that it fails because the author pushed a new commit, it will comment (only once): Review required! Latest push from author must always be reviewed before requiring reviews again.

I hope that this makes it clear for new users.

[bkchr]

Ty!

[bkchr]

"Review required!" is not really such a generic sentence that could not come from someone else :P

Please either search for the full comment or filter by the bot as author?

[Bullrich]

Good call, I applied the whole comment as the text to search for.

[mordamax]

@Bullrich i saw that it might call Review required even for push with PR without any prior approvals given. Would that be expected?

I think if yes, then it would "soft-force"people to use draft mode more often if the pr is WIP

[Bullrich]

@Bullrich i saw that it might call Review required even for push with PR without any prior approvals given. Would that be expected?

I think if yes, then it would "soft-force"people to use draft mode more often if the pr is WIP

It is expected, as it checks for new pushes only.

Do you have any suggestion in how could we soft-force people to use draft? That would be the ideal scenario.

[mordamax]

@Bullrich i saw that it might call Review required even for push with PR without any prior approvals given. Would that be expected?
I think if yes, then it would "soft-force"people to use draft mode more often if the pr is WIP

It is expected, as it checks for new pushes only.

Do you have any suggestion in how could we soft-force people to use draft? That would be the ideal scenario.

That's what i meant - i think the current logic already does it

check this PR for example: #3431 (comment)
It looks like it's WIP as there are many commits added while it was NOT in Draft, and NO approvals before..

Now after this bot message -

1. there's almost instant approval
2. probably, if it still gonna SPAM further with the same message, Muharem would move it to Draft till the work is really done.

So I have assumptions:

1. It could be SPAMMy if developer need some CI to test their stuff (which won't work in draft I suppose unless you publish and move to draft again)
2. may be better to send such message only if before there was more than 1 approvals?

[ggwpez]

Donal is part of core devs but we just got disapproved here: #4099
Is this expected?

[mordamax]

Interesting but some of the members (like here @seadanda) are recognized as CONTRIBUTOR, not a MEMBER
#4099 -> https://github.com/paritytech/polkadot-sdk/actions/runs/8692055614/job/23835753129

seems like it should be github.event.pull_request.author_association != 'CONTRIBUTOR' && github.event.pull_request.author_association != 'MEMBER' per https://michaelheap.com/github-actions-check-permission/ & https://stackoverflow.com/questions/63188674/github-actions-detect-author-association

[mordamax]

@Bullrich #4137

[bkchr]

@Bullrich i saw that it might call Review required even for push with PR without any prior approvals given. Would that be expected?

Really? I thought that this would only happen after the pr was already approved once and then there was a new commit... I should have read this stuff more carefully. @Bullrich please remove this code until it is improved.

[Bullrich]

@Bullrich i saw that it might call Review required even for push with PR without any prior approvals given. Would that be expected?

Really? I thought that this would only happen after the pr was already approved once and then there was a new commit... I should have read this stuff more carefully. @Bullrich please remove this code until it is improved.

Found associated fix in #4152. This will ensure that it contains at least a single approved review before triggering.