Marvin v3: Tracking a PR's lifecycle

[SigmaSquadron]

Issue description

In 2022-04-16 at 12:48:13 UTC, Marvin was declared dead. For those unaware, Marvin was an experimental GitHub app that tagged Nixpkgs PRs and was used by maintainers and committers to triage PRs. New PRs would be tagged with awaiting_reviewer, then awaiting_changes after a review has been posted, and finally awaiting_merger after an approval.

Last year, the old Marvin labels began to be manually used in PRs. This somewhat polluted Marvin's historical log, and isn't an ideal solution, considering how the labels are undocumented and using the default #FFFFFF scheme, which is usually reserved for 5.scope labels. This clearly shows the need for a new version of Marvin: a PR lifecycle tracker that clearly displays which PRs need reviewers, have been awaiting reviewers, or are waiting on the PR author for changes.

Thanks to the power of hindsight, we can avoid Marvin v2's biggest issue: the spam. Marvin would request reviews, ping maintainers, and leave superfluous comments in the PRs it managed. I propose a simple labeler that does nothing but automate the work that @FliegendeWurst has been doing, which will allow us to filter PRs more effectively. This also allows us to design a much more simple solution with far less features.

Proposed solution

Leverage GitHub Actions or an App to add the following labels to PRs:

4.stage: awaiting reviewers, for when a PR has no reviewers after nix-owners runs.
4.stage: awaiting review, for when a PR has review requests, but no published reviews yet.
4.stage: awaiting changes, for when there is any published review with unresolved comments. (TODO: can we track resolved/unresolved review comments?)
4.stage: awaiting merger, for approved PRs which have not been interacted with by a committer.
4.stage: awaiting merge, for approved PRs which have been reviewed by a committer or have requested the review from a committer. Both of these will mimic the 12.approvals series of labels.

Extra: move unresolved_build_failure to 2.status or 9.needs. I'm not sure if that's Marvin-related, but it sure looks like it, and it could do with a touch-up.

This list is not final and is always up for discussion and improvement. It's a more-or-less 1:1 replacement for Marvin, but it could be argued that awaiting merge(r) doesn't need to exist now that 12.approvals is a thing. I'd also be up for adding more labels for more PR states, such as when the changes were addressed but no approval was given yet.

There should also be a way to opt-out from labeling, and another label from when the usual lifecycle is skipped. For instance, a PR like this could be given a 4.stage: reviews skipped label, which would allow us to track trivial, tested PRs from package maintainers, and perhaps slightly improve breakage tracking if someone has been reckless.

---

Add a 👍 reaction to issues you find important.

[FliegendeWurst]

I would propose renaming "4.stage: awaiting reviewers" -> "4.stage: needs reviewer" (to mimic 9.needs: maintainer) and maybe giving it a slightly different color. Unlike the other stages, reaching this stage did not ping anyone.
Also I would delay applying that stage until both nix-owners and eval pass, since both ping reviewers.

TODO: can we track resolved/unresolved review comments?

The API docs are here. They make no mention of resolved comments.

Leverage GitHub Actions or an App to add the following labels to PRs:

I am not sure GHA can respond to all the relevant events. Probably the easiest solution would be to use the API directly with a bot account.

And you are right that the merger stages aren't really needed with 12.approvals: *. We should also automate those label too.

[wolfgangwalther]

In #402332 I added some labels in the 4. category, which I called 4.workflow:. I guess we could either fit the here-proposed labels in there as well or use a different number.

[SigmaSquadron]

No worries. There's always 13.* and beyond.

[wolfgangwalther]

Last year, the old Marvin labels began to be manually used in PRs. This somewhat polluted Marvin's historical log

Is this historical log important? In #419944, I propose to just delete them.

[SigmaSquadron]

I agree with that proposal. There's no longer any reason for them to be here.

[wolfgangwalther]

Leverage GitHub Actions or an App to add the following labels to PRs:

Once #419878 is merged, we have a good base to start adding new labels in the new labels.yml workflow. We manage merge-conflict, stale, approvals and rebuilds already.

I am not sure GHA can respond to all the relevant events. Probably the easiest solution would be to use the API directly with a bot account.

That's what we do now - every PR is looked at roughly once a day. And within 10 minutes, if anything in that PR changed. So that should work well here, too.

4.stage: awaiting reviewers, for when a PR has no reviewers after nix-owners runs.
4.stage: awaiting review, for when a PR has review requests, but no published reviews yet.
4.stage: awaiting changes, for when there is any published review with unresolved comments.

Those sound like they are a doable.

[pbsds]

4.stage: awaiting changes, for when there is any published review with unresolved comments.

What if the pinged reviewer uses a comment and not a review? This is quite likely since it requires two fewer clicks and a page reload less.

Should any comment by a pinged reviewer count as a review? We could make a 👍 reaction by the "reviewer" or PR author count as it being resolved.

I fear also that nit reviews will with this automation make otherwise correct PRs far less discoverable.

[wolfgangwalther]

Thinking about it some more, there is a gap between these two:

4.stage: awaiting review, for when a PR has review requests, but no published reviews yet.
4.stage: awaiting changes, for when there is any published review with unresolved comments.

So, I got a review, the state changes to "awaiting changes". I address those... what will the state now become? I'm not going to approve it as the author, I need a reviewer to come in (again). So the state should arguably be "awaiting review" again (?).

Aka: Every PR must be, per definition, in one of the 3 states "awaiting reviewer", "awaiting review" or "awaiting changes". "awaiting reviewer" is pretty clear, but the other two must be defined in a awaiting review == ! awaiting changes relationship.

Thus, I would argue, that one of the two labels is not required. I would drop "awaiting changes". So, a PR either has an "awaiting review" label - or it doesn't.

"awaiting review" should be defined as:

• there are reviewers requested
• all given review comments have been resolved

So it would be set initially, when no reviews have been given, but it would also be set once the changes have been made.

Should any comment by a pinged reviewer count as a review? We could make a 👍 reaction by the "reviewer" or PR author count as it being resolved.

I feat this would make it incredibly error-prone. If somebody comments without giving a review, the worst thing that happens is.. that the PR keeps the "awaiting review" label. Which is not bad, it only invites more reviews.

I fear also that nit reviews will with this automation make otherwise correct PRs far less discoverable.

If we do NixOS/org#122 (comment), we might make a distinction between "requested changes" and... everything else. Everything that is not "requested changes" is automatically a nit.

I can't find a reasonable way how to take this into account, though :/

[pbsds]

(this was written concurrently with the comment above)

I was recently impressed by the gitea label bot in go-gitea/gitea#34857. It manages merging requirements not review state. With something like it achieving https://delroth.net/posts/nixos-security-wishlist/#two-people-rule-for-nixpkgs-merges may become possible.

I also think it may make sense to rephrase the 4.stage: * "state machine" to instead be a union of met conditions, meaning the labels are not mutually exclusive. This makes it so contributors and reviewers don't have to fight/babysit them as much.

What about?

• 4.review: needs reviewers, for when a PR has no reviewers, meaning no one has it on their radar.
• 4.review: needs review, for when a PR has review requests, but no published reviews by anyone or comments by those pinged. This could even be relaxed to simply having to be interacted with by anyone other than the author.
• 4.review: needs changes, for when there is any published "request changes" reviews with unresolved comments. I prefer the needs to be somehow opt-in by the reviewer, not the default. We should maybe also consider making any (force)push "address" the review to avoid false labeling.
• 4.review: needs committer, for approved PRs which have not been interacted with by any committer. The moment a committer has interacted with the issue we can assume they're subcribed. This mirrors my go-to 5kb-url search deselecting involvment from any committer
• 4.review: awaiting merge, how do we even programatically track this?

[wolfgangwalther]

I like 4.review: needs reviewers and 4.review: needs committer. I would say the logic should be: is any of the listed (potential) reviewers a committer? This would allow any committer to request their own review at some point, even if they didn't do anything, yet, to just signal "I am planning to drive this PR eventually".

I would not go as far as "any interaction", though. I quite frequently interact with PRs after breaking CI :D to clear up the mess that I caused. I sure wouldn't want to count as "the committer" for all of those.

I'm not sure about the other two (neither your proposal, nor mine, really...).

[wolfgangwalther]

would say the logic should be: is any of the listed (potential) reviewers a committer? This would allow any committer to request their own review at some point, even if they didn't do anything, yet, to just signal "I am planning to drive this PR eventually".

Ah, let me quickly add to this: This won't work. There are far too many review requests from committers who never react in any way. Maybe the logic should be "needs committer is removed once any committer has given any kind of review".