"Reviewed by team last at commit <commit>" checkbox for team mentions

[blyxyas]

I'd love for the subtree change detection comment to have a "reviewed by team" checkbox, that unchecks at every new commit.

Current

The Miri subtree was changed

cc [@]rust-lang/miri

Some changes occurred in src/tools/clippy

cc [@]rust-lang/clippy

Proposed

The Miri subtree was changed

cc [@]rust-lang/miri

• Team review ([current commit]) by ...

Some changes occurred in src/tools/clippy

cc [@]rust-lang/clippy

• Team review ([current commit]) by ...

---

I receive lots of pings of issues that don't necessarily change something important in Clippy (pretty much every change to structs in rustc_hir or rustc_ast also needs some very light adjusting in Clippy). So by just a quick glance, everyone on the team could know that someone else on the team has reviewed it, or if they're the first one to check the mention.

[Urgau]

Currently each mention is only triggered once, would your proposal require a new mention/ping for each new commit? How would that handle force-push?

---

The mention message can be individually customized with a custom message body, you could manually include the checkbox for your mention (it wouldn't give you who reviewed it, but I'm not sure how triagebot would know that as well):

 [mentions."src/tools/clippy"]
+message = """
+The clippy subtree was changed.
+
+  - [ ] Reviewed by a clippy maintainer
+"""
 cc = ["@rust-lang/clippy"]

[Urgau]

it wouldn't give you who reviewed it, but I'm not sure how triagebot would know that as well

I guess we could introduce a special symbol/annotations on the checkbox line that would be replaced when the checkbox is being checked/un-checked, I'm thinking of something like this:

  - [ ] Reviewed by a clippy maintainer <!-- TRIAGEBOT_CHECK_HANDLE -->

and when it's checked by someone would become:

  - [X] Reviewed by a clippy maintainer <!-- TRIAGEBOT_CHECK_HANDLE_START --> @blyxyas
 <!-- TRIAGEBOT_CHECK_HANDLE_END -->

The same system could handle many other things like current commit, datetime, ...

This is of course just hypothetical and would require motivation from T-clippy (or others) given the complexity it would be to implement and maintain.

[blyxyas]

Currently each mention is only triggered once, would your proposal require a new mention/ping for each new commit? How would that handle force-push?

I’m not sure how Github handles edits on mentions, the idea isn’t that we post new comments, just that we edit the one we made on the comment by unchecking the box and removing the reviewer. Although I see the complexity in doing that. Force pushes wouldn’t be handled AFAIK, as the official rustc-dev-guide states that changes must be done as normal commits unless they’re rebases (i.e. not that important) or squashes (i.e. the same code)

—-

About getting the reviewer, the Github API has a get-reviews endpoint for every PR and we can diff that against the team repo information.

[blyxyas]

Also, I’ll talk with people on Zulip if this is something that they’d be interested in

[Urgau]

the idea isn’t that we post new comments, just that we edit the one we made on the comment by unchecking the box and removing the reviewer

That would feasible. We have the infra to edit comments.

Force pushes wouldn’t be handled

So in your example, what would the current commit part point to?

Note that in general we don't distinguish between a force push and just new commits, we handle them the same way in [mentions] by diffing the entire PR each time and pinging the people/team we didn't before.

[blyxyas]

The commit part isn’t that important, not as much as it being resettled in every new commit. If rebases aren’t handled differently, I guess it’s fine? I guess we could ignore the new commit if we have event information that matches either Event::HeadRefForcePushed or Event::BaseRefForcePushed (not documented) heuristically.

[Urgau]

One question that arises regarding the reset of the review, how do you expect members of T-clippy (in our example) to know they need to look again at the PR? Do you expect the members to look at the mention comment every time a push is made to see if it's being reset?

I think the simplest way would be to not have a reset, but instead issue a new mention comment with the new commits that touches clippy, that way any T-clippy members would immediately know which commits needs to be reviewed (and if it's already being done), and if there are no new mention comment that means there is nothing to be done. (this would also be much simpler for triagebot to implement)

Btw, regarding the webhook

I guess we could ignore the new commit if we have event information that matches either Event::HeadRefForcePushed or Event::BaseRefForcePushed (not documented) heuristically.

Those don't look like webhook events, you need to look at the webhook pull_request event for the "action" available: https://docs.github.com/en/webhooks/webhook-events-and-payloads?actionType=synchronize#pull_request.

Webhooks only provides a synchronize "action" for PRs, which is not enough to know if it's a force-push but we can use it to detect force-push by finding the merge base commit of the new and previous head PR commit and comparing them. (It's convoluted but we already do that, not that I want to do it else-where).

[Urgau]

@blyxyas any news on your side?

[blyxyas]

Not really any news, I'll close this and it can be reopened if desired.