We're interested in this option, since we find that users are often surprised by the existing behaviour. I think this would make sense as a per-package setting, rather than a global one.

The use case in this task's description does not make sense to me.

In this scenario, user A is an expert on X. User D is an X novice.

You're saying it's okay when D authors a change to X and A reviews it, but not okay when A authors a change to X and D reviews it? Why is A's expertise less trustworthy when they're an author than it is when they're a reviewer? That seems entirely backwards.

When you put it that way, it does seem backwards.

A couple of other angles/criticisms of the current behaviour:

• Sometimes you want to maintain an invariant that "all changes to X have gone through Y review process" (perhaps for ~compliance~ reasons). In this situation, it doesn't matter whether the author is qualified to perform the review; the review must take place regardless.
• As owners adoption becomes pervasive, we're imagining that the standard workflow won't involve explicitly adding reviewers during arc diff; instead, the owners system will automatically add the "right" reviewers to each diff. But if the diff only touches a package (or packages) where the author can approve reviews, there will be no reviewers added at all and the diff goes into "Waiting on Review" limbo. (This is pretty speculative, so perhaps it's not a strong reason for changing the author-is-owner behaviour.)
• Our users are frequently surprised by the current behaviour. "Why didn't <diff> get the reviewer added?" is a fairly common question, and the review UI doesn't make it obvious what happened.
  • Most of our users don't configure owners in Phabricator directly (we're syncing in information from OWNERS files within the codebase), so it's possible that we could try to address this by improving our internal documentation. But we do already mention this quirk, and it rarely seems to stick in users' minds.
• The current behaviour means that someone who just configured an ownership/auto-review rule can't confirm that it's working by posting a diff.

As more of a point of philosophy of code review also, I (and, I think, my teammates) would say that diffs should be reviewed by someone who both:

1. Understands the relevant code
2. Did not write the code themselves

In other words, the combination of expertise + different perspective is of value beyond each component individually. If you believe that, then B or C should review A's diff in this case.

Obviously this point is entirely a matter of opinion, but I think this is why many users expect to see the group review added even if the author is in the group.

For the compliance case, you should already be able to trigger this stronger rule ("always add the package as a reviewer, even if an owner authored it") with Herald, by writing this rule:

When:
[ Affected packages include ] [ Compliance ]
Take these actions:
[ Add reviewers ][ Compliance ]

The documentation alludes to this, although it does not call this scenario out by name:

The intent of this feature is to make it easy to configure simple, reasonable behaviors. If you want more tailored or specific triggers, you can write more powerful rules by using Herald.

Perhaps a case of T8644.

The cost of this is that you have to write one rule per compliance package, but "hopefully" you only need to comply with a countable number of Very Important Compliance Rules and this is reasonable. (If you have, like, 20+ distinct "compliance" packages, I'd be interested in learning more about what you're complying with and why.)

For the "suggest reviewers" case, I'd like this to happen at arc diff time (T10622 roughly talks about this -- I think there was a better task somewhere but couldn't immediately dig it up). If it happens at Herald time, the author can't preview what will happen or adjust the list, and can't see that they don't need to add more reviewers. That is, if Herald does this, we get this flow:

• arc diff
• See "Reviewers:".
• Can't tell if you need to do anything?
• Even if you don't, arc still complains that you didn't add reviewers if you save-and-exit without touching the field.

If it happens before arc diff and you see Reviewers: O123 Compliance, O245 Extra Compliance, you can make a better decision about if you need to add anyone else and/or who to add. T2443 is also somewhat related.

I'm not totally sure about how/why users are expecting the behavior in the third case. Are they doing something like this (basically, wanting (2) to work)?

• arc diff
• See "Reviewers:".
• Save and exit without adding more reviewers.
• Wishing that did what they wanted so they didn't have to type anything?

If they did that, and expected it to add a specific package, and knew that package by name, it seems like this should be confusing only once until they learn the rule, and then fairly clear? Maybe not ideal, but not like an ongoing trap, and probably better resolved by suggesting reviewers at arc diff time so they can see what will happen.

(I think this isn't really feedback we've seen from other installs -- maybe we've just missed it, but maybe there's some kind of context or expectation that's a little different in your case.)

We could possibly address (3) and (4) by listing "Packages Which Would Have Been Added as Reviewers, But The Author Is An Owner Already So They Weren't: X, Y" in the UI somehow. They're semi-listed in the "Packages" column in the table of contents, but you have to have a fairly deep understanding of things to know/trust that. I don't think this is entirely unreasonable, but it would be an element that was useless 99% of the time.

We could also add a feature like "Test if a revision would trigger review by this package" where you type in Dxxx and it says "yes" or "no" or something. Or we could document "Go look at the Table of Contents to predict the effects of auto-review". But none of these feel too great offhand.

I'm not particularly strongly opposed to adding more options here, I just want to avoid the dropdown becoming this:

Auto-Review: [ Subscribe to Changes ]
             [ Review Changes ]
             [ Review Changes (Blocking) ]
             [ Review Changes, Even if Authored by Owner ]
             [ Review Changes (Blocking), Even if Authored by Owner ]
             [ Suggest Reviewer ]
             [ Suggest Reviewer (Blocking) ]
             [ Suggest Reviewer (Not Removable) ]
             [ Suggest Reviewer (Blocking, Not Removable ]
             [ Suggest Reviewer, Even if Authored by Owner ]
             [ Suggest Reviewer Unless the Moon is Full Or Author is Subscriber or Owner is Blocking Reviewer (But Unsubscribe) ]
             ..

Or a big list of checkboxes which amount to the same thing, particularly since you can already implement a far more flexible superset of these rules in Herald. At some point, we just end up embedding a bad, inflexible version of Herald into this UI.

(I think T11118 is also roughly the same task but about audit instead of review -- I think there were some slightly stronger use cases there, maybe, and if we add that feature it would bolster the case for adding equivalent "Auto-Review" options.)

I'm not totally sure about how/why users are expecting the behavior in the third case.

I think it's largely that users form a mental model of "changes to X get reviewers automatically added by the owners system", without the caveat about the author.

Another potential reason for surprise is that project members don't always get updated as promptly as one might hope. It's natural to add a user to a new project when they join a new team, but it's easy to forget to remove them from an old project. After a while, their expertise may be stale, even though they're still exempted from the auto-review setting, perhaps unknowingly. (Indeed, users must now *explicitly* accept "on behalf of" an owners package, but the author exemption happens without the user opting in--and silently, without any option to opt out.)

@sakura points out another case, with the "auto-subscribe" setting. If that setting is configured, it's likely with the intent of keeping every owner informed of every change to the package. Probably everyone would like to stay informed, regardless of whether or not the author is an owner.

New, far wordier options available at HEAD: