bug#71622: mumi CLI client features for review checklists

[Suhail Singh]

Arun Isaac <arunisaac@systemreboot.net> writes:

> # Idea 1
>
> Projects provide a review checklist in their .mumi/config.

I think idea #1 can add considerable value.  I'll note some opinionated
observations below.

> For example, something like
>
> ((review-checklist . (((name . good-commit-message)
>                        (description . "Are the commit messages written well?")
>                        (tag . review-good-commit-message))
>                       ((name . good-synopsis-description)
>                        (description . "Are the synopsis and description 
> written well?")
>                        (tag . review-good-synopsis-description))
>                       ((name . tests-run)
>                        (description . "Are the package tests being run (if 
> available)?")
>                        (tag . review-tests-run))
>                       […]))
>  […])

I think the fact that the names are distinct from the actions (the tag
that is to be added) is quite important.  I.e., the "review-checklist"
should provide a convenience around "well-defined state transitions"
where the state is being recorded by usertags, but could also be
extended to include issue status (open, close, owner etc).

For instance as noted in
<https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#4._Set_yourself_as_the_reviewer>:

- When setting yourself as the reviewer, ensure that no-one else has set
  themselves as the owner.
- Assuming above is true, when adding under-review tag, also add
  yourself as the owner.

On a related note, perhaps "review-workflow" might be a better name than
"review-checklist"?

> When a reviewer checks one of these items (say the good-commit-message),
> they run something like
> $ mumi review --tick good-commit-message
> and that sets the review-good-commit-message tag on the issue.

It may be important that when a tag is set, that others are unset.  It
would help if this were possible to express via the review-checklist.
For instance as noted in
<https://libreplanet.org/wiki/Group:Guix/PatchReviewSessions2024#12._Change_the_issue_state>:

There are three possible transitions after a review is completed:
- Addition of reviewed-looks-good, removal of under-review .
- Addition of escalated-review-request, removal of under-review .
- Addition of waiting-on-contributor, removal of under-review .

The user-interface (as well as the configuration) should focus on those
transitions.  Some transitions may indeed be simple and may only result
in the addition of a single tag, whereas others may be more complex.
Additionally, given the current state, only some transitions may be
"sensible" and permissible, i.e., it would help to be able to express
conditional/guarded transitions.

> One possible downside is that this ties each project (guix, mumi,
> etc.)  to a single checklist.

I think that that's actually a benefit.  It can aid review of the
transition rules for them to be in one place.

> For example, what about guix patches that are not for packages?
> Perhaps it is an idea to allow multiple checklists per project.

One way to address this would be to have the ability to have multiple
conditional "checklists" (or workflows) that are mutually exclusive and
exhaustive.  I.e., support for nested "checklists" in addition to
guarding would be sufficient, but may not be necessary.

> Another downside is that this does not provide for multiple reviewers to
> review and verify each other's findings. In other words, there is no way
> for two reviewers to register that they both verified something
> independently.

Could that be addressed by ensuring that the control messages are also
cc'd to the specific issue?

-- 
Suhail