Page MenuHomePhabricator

Would like herald rules that add required reviewers be more of a guarantee
Closed, DuplicatePublic


My company is in the process of moving to Phabricator and discarding custom pre-receive hooks that enforce reviewers in favor of using Owners+Herald rules that add blocking reviewers to Differential that enforce the same rules.


We wanted the Herald rules to be a guarantee that if a revision is "Accepted" all of our review requirements have been met, but we've noticed that users are able to remove Herald-added required reviewers (e.g. via the "Edit Revision" dialog). The reviewers will be put back by Herald, but only if the user updates the diff again (T10109), making it possible to accidentally push changes that haven't gone through proper review.

To be clear, we're not worried that bad authors are trying to intentionally circumvent the rules, but we have users who regularly edit Reviewers (the ones that weren't added by Herald) and while they're there we've seen a few instances of them also dropping a required reviewer (especially because Herald doesn't remove reviewers if the revision changes such that a rule's conditions are no longer true, so users are used to occasionally needing to manually remove required reviewers).

We'd like to be more guaranteed that the "Accepted" status actually means our review requirements have been met. We could probably do this by having a pre-receive hook that somehow tricks Herald into running and then checks the status of the revision only after it has run, but we'd prefer that authors can be sure that if the Phabricator UI says "Accepted" the push won't be rejected.


From T10109 it sounds like triggering Herald on every change to a revision is not desirable, but it might be okay to make specific types of updates trigger Herald?

If that's the case it seems like triggering Herald rules on revisions any time the set of reviewers changes would make Herald rules that add reviewers be a guarantee that those reviewers are set if the rule's conditions are met.

If this would be an acceptable change I'd be happy to try submitting a diff for it, it looks like something similar was already done to trigger Herald rules if someone commandeers a revision so that author-based legal signatures requirements can be set correctly.