Requesting new review of a non-owned Diff via conduit

Good day,

I have a question for which the conduit web console, Google and this discourse instance did not seem to hold any answer:

How could one, using conduit, request a new review of a (previously accepted) Diff, with the diff belonging to an arbitrary user?

Conduit has the differential.revision.edit method for which there is the request-review transaction. Unfortunately, when called for a non-owned diff, this yields:

{
  "result" : null,
  "error_code" : "ERR-CONDUIT-CORE",
  "error_info" : "Validation errors:\n  - You can not request review of this revision because you are not the author of the revision and it is not currently a draft."
}

The use case is a webhook that runs a few checks when diffs are updated, and which should trigger a new review under certain conditions (eg, have the same effect as if the user did the Request review action on their diff): is there any proper way to do this that I missed?

A work-around that is likely satisfying is to remove and re-add some reviewers(*), but it left me wondering if I missed anything.

Have a pleasant week!

(*) this seems to have the added benefit of having the diff only pop into the queues of those that need to re-review.

I’m not clear on your use case. After an update, I expect that a revision is normally in the “Needs Review” state. The “Request Review” action just puts the revision into that state, so I would expect that your webhook will not normally have any effect (it will usually move a “Needs Review” revision into the state “Needs Review”).

Can you step back a bit and explain what your webhook is doing more generally?

If your goal is to remove the “Accepted” state, a cleaner approach may be to:

  • disable differential.sticky-accept; and
  • have your webhook do the opposite (“Accept” revisions as a bot user named “Trivial Update Approver” if the update satisfies your rules that permit sticky accepts).

There is no way to “un-sticky-accept” a revision after the fact as a non-author user. You can remove and re-add reviewers or commandeer the revision, request changes, then give it back to the author. Both sets of actions have other side effects beyond the effect of “Request Review”.

Support for “sticky by default, then a bot un-stickies revisions” is unlikely to improve in the upstream because this is not safe-by-default: it creates a window where the revision is in a too-permissive state (“Accepted” when it should be “Needs Review”, in the time between when the revision is updated and the bot acts). If a customer use case arose in this vein, I’d likely pursue safe-by-default changes (possibly in the vein of a “Trivial Update Approver Bot”) instead.

Thank you for the reply.

The goal of the webhook is precisely to “un-sticky-accept” certain diffs:

Our use case

We are happy to have sticky accepts for the vast majority of diffs (even if non trivial), except for a few very specific directories that belong to particular package owners:

When such directories are touched, the diff should become “non-sticky” to avoid erroneous or malicious updates after the diff was accepted.

The webhook essentially checks:

  • if non-sticky package owners are involved
  • if so, if said owners have accepted the diff since the sources were last updated
  • if not, get the diff back into “needs review”, at least from the perspective of the package owners.

Regarding the safety issue: to avoid any problem with the “window of opportunity” (which could indeed be substantial if the webhook is down for a while), a mitigation we though of involves a pre-receive commit hook which calls the webhook as well: its response payload would tell the pre-receive-hook if the current state of the diff is acceptable according to our custom rules. And if the hook is currently down, nothing can be pushed.

Inverting the logic

I admit we did not consider the option of “inverting the logic”, which certainly has safer properties and looks cleaner: however, differential.sticky-accept being an instance setting, and as the ‘non-sticky’ use case is only for a very few particular repositories, I’m not sure if it would end up being “simpler” in our particular case. (edit: but given some hoops we need to jump through, that solution certainly has some appeal).

Finally, I believe we’ll find an acceptable solution with what is available: this is certainly not a feature request(*), I just wanted to check if we had missed anything. As herald takes care of adding back any obligatory reviewers, removing said reviewers through conduit causes the desired effect for us.

Many thanks and kind regards,
Julien

(*) actually, if I was to be granted a Christmas wish, I’d love to have a feature where conduit would let me ask “for Diff X, please rescind the accept of owner or group Y”.

Slightly related question:

as a mitigation to this issue I’d like to use conduit to add a blocking reviewer under certain conditions.

However, while differential.revision.edit give us the reviewers.add transaction type, I can’t find anything there related to blocking/non-blocking reviewers.

I tried to prefix the phid with a ! to no avail. Is there an ‘alternative phid’ that would represent a user/project/group as a ‘blocking’ reviewer instead of a ‘vanilla’ one?

Pass the string blocking(PHID) instead of PHID:

Screen Shot 2021-01-21 at 8.08.33 AM

pray