This is the most awesome feature request I've ever read. Well stated, sir.

Do you think the majority of these blocking reviewer rules are good, or is figuring out why blocking reviewers were added more-than-occasionally the first step in removing them? If these rules added normal reviewers -- instead of blocking reviewers -- would this be a problem?

My product expectation is that "blocking reviewers" will be used very rarely, so it's surprising to hear that use is so common that it's hard to figure out where blocking reviewers came from. Offhand, I would expect <5% of revisions in a "normal" environment to trigger blocking reviewers.

One product change which might be appropriate is to restrict who can create "blocking reviewer" rules if they're prone to abuse in the wild, although ideally this should be self-regulated / cultural. But, philosophically, the goal of all the tooling on top of this pipeline is to put the onus on reviewers to review promptly if they want to have a say in changes, not let them add rules to block everything.

So, to step back, do you think the use of blocking reviewers is currently appropriate/desirable in nearly all cases and the best solution is definitely to improve the tooling, or is the use of blocking reviewers sometimes questionable and improving the tooling is just a mitigating step?

Or, put another way: what actions do you take based on this information? That is, once you figure out why blocking reviewers were added, what do you do next?

I also get this error when following the Chrome extension link:

In particular, at Facebook:

• We didn't have "Blocking Reviewers".
• I built, then removed "Add Reviewer" because I felt it was being used way too often. Revisions would often end up with 4-5 random reviewers and the sense of who was responsible for review would be lost. I think this was a beneficial change on the balance.

So, at least when I left in 2011, you could only add yourself as a subscriber to things you were interested in, not a reviewer. This worked well, even at Facebook's scale, and I think we were better off for the removal of the "Add Reviewer" feature.

We later restored "Add Reviewer" because users frequently requested it or found it confusing that it wasn't available, and didn't find my old war story about questionable use very compelling, and I got tired of telling it. I think this feature might make the product worse on the balance, but giving installs enough rope to shoot themselves in the foot makes the product much easier to support because no one finds "one time I saw it get misused so I'm pretty sure no one can handle it" to be a very convincing argument.

Definitely lots that can be done to improve the ui here, but I'm most excited to learn what you do once you figure out why a given set of reviewers was added. (...I hope there's drama sometimes!)

So I guess I should explain more about our use case.

As far as I am aware, we only have 2 groups of blocking reviewers (we used to have a 3rd for schema changes, but that's been replaced by an automated schema validator). They are Security and Availability. The latter is pretty straightforward - generally triggers on nginx config changes, which aren't terribly common. Security, however, has a ton of herald rules to catch changes to security-sensitive files and review uses of otherwise dangerous functions. I believe the fact that we have a security review process is part of our SOC2 compliance, so it isn't going anywhere soon They are making efforts to limit what requires security reviews (e.g. by providing always safe abstractions in place of unsafe functions whose use needs to be reviewed, and better lint rules to warn about e.g. xss vectors), but fundamentally security reviews aren't going to stop being a thing anytime soon.

The heraldsplainer chrome extension was the Security team's hacky way of making security review easier on themselves. A lot of things trigger security review that are just uses of security abstractions - like using a certain unsafe function for redirects, or fetching our csrf token. Sometimes security reviews are triggered for spurious reasons, like mentioning a security-sensitive function in a comment (which arguably shouldn't trigger security review, but herald isn't that smart), or rearranging existing imports. In most cases, it's not really reasonable for the security reviewer to read a whole 500 line diff if there's only 1 line that herald thinks is important to them, so it's helpful if they can very quickly identify why herald flagged it and just focus on that. Since a lot of things trigger security review, the security reviewers need to be able to go through outstanding reviews quickly to ensure timely responses for everyone.

Heraldsplainer isn't really the best solution, though, as not everyone is going to install it, and security engineers aren't the only ones who want to know why security is a blocking reviewer on a review. Having this info natively in phabricator would be preferred.

It sounds like lint is a much better fit for this problem than Herald (for example, it won't raise warnings in comments, and it points out exactly where issues are, engineers get a chance to see issues before they get sent for review, etc) -- are there reasons it isn't that I'm missing, or reasons that moving these review rules to lint rules has been slow or impossible?

there are plenty of cases where we do want a security review instead of
just lint warnings - it's definitely not as simple as converting everything
to lint rules. And part of the problem is that lint isn't expressive
enough - and in some cases can't possibly be.

Here's one such example: we have an unsafe_redirect function (which
triggers security review) which can redirect to almost anywhere (the
preferred option is to use redirect, which checks that the input url has
a domain on our safe list). The general advice is that unsafe_redirect
should be given a constant url, or at least a url with a constant domain.
But not all use cases can use constant domains, and security definitely
wants to review these before they are committed.

Suppose it worked like this:

• Lint flagged the line with a warning, like "(SECURITY) This unsafe_redirect call may be unsafe, see [some documentation] to learn more about redirects.".
• Herald had a rule like "When any lint message text contains (SECURITY), add blocking reviewers: #security".

Then you'd still be a blocking reviewer, but all the security stuff would be in the "Lint warnings" at the top of the page, and you could click to jump to the exact lines where the issues occur. The lines would also be annotated inline by the lint messages, too, so you could quickly scroll through the diff and see where the problems are.

Setting aside the complexity of actually moving the rules to lint, would that be a better product interaction? Just want to make sure I understand the problem.

that would be nice (maybe even ideal) if lint was actually a reliable thing
for us.

Context: I've seen various issues in the past where linters aren't
installed properly and end up silently not running. And since we don't run
them serverside but only on dev vms, and can't guarantee vms all have the
latest linters... I'm not sure this is really a workable solution. Though
I should probably ask our dev vm guys what they think about it.

Overall, we can definitely improve the transcript view, but it will never be an especially good solution to this problem:

• Herald can not realistically do real parsing, so it will always misfire for comments and can not implement complex rules (for example, in this project we have some security rules which require specific parameters to be statically evaluable scalar strings, but you can't really do this with a regular expression).
• Knowing which rules fired is helpful, but it won't pinpoint where in the diff the problems are.

We'd rather tackle the lint problems directly, e.g.:

• Provide better ways to make sure the right linters run.
• Provide any infrastructure required to have linters make sure they're up to date.
• Server-side lint will eventually be available through Harbormaster (T1049), although it is inherently more limited than client-side lint because it can not emit patches to automatically fix code. But, for the security use case, this would ensure that lint rules run.

Some changes to Herald which would make your use case better (like being very verbose about which rules matched, by default, in a highly visible way on the revision itself) would make more common use cases worse (by adding a lot of information which is usually low-signal in a prominent place), but improving linters makes things better for everyone.

One hacky workaround might be to create a project for each type of security issue and have the rules both "Add Blocking Reviewers" and "Add Project". This would let you examine the projects to see tags like "Security: unsafe_redirect", which would give you approximately as much information as knowing which rule matched. There's currently no Herald action for this, but it is approximately described in T6973 and not difficult to implement. The roughness of this solution would also only impact your install, rather than creating a UI tradeoff.

We're meeting at Dropbox next week -- I'm not sure if you're involved, but we can try to follow up on this. We expect to have more ability to prioritize requests after that.

hey! So I am in the security team at Dropbox. Would you have time to chat with some of us when you are at Dropbox next week? A lot of this might be easiest to figure out IRL.

Michelle Sordal owns the agenda / schedule. Any dropbox folks should shoot her a note to get included on the action...! :D

+1000 to improving serverside lint support. I have no problem with "can't
auto-change code" as the few uses of that we have have drawn some
complaints.

As far as meeting with people here, Dev is the right person for you to talk
to - I was raising this as a feature request mostly as a "we have a chrome
extension for what?" complaint. And also because Herald logs are fairly
hard to read.

I really enjoyed reading this comment thread! :)

I think reliance on lint for Security enforcement is probably blocked on T5064. Otherwise, someone could accidentally add an unsafe method after the code is accepted, but before they arc land, and lint would never catch it.

sweet. I am sure we can work with those. I think Herald Actions in itself should work pretty easily in our existing workflow. I am very excited about server-side lint, but a bit more documentation about them would help me out. I can't understand it fully with your comments :)

Briefly, T9529 discusses some server-side system running on revisions that's live in your environment, presumably. Its current behavior is something like this:

• Be told about a new revision (by "Changes", I think)?
• Run some-coverage-program on the code.
• Upload the coverage results with harbormaster.sendmessage (in unit messages).

Just add two more steps at the end (or copy/paste the whole thing or whatever, I have no idea what the system actually looks like):

• After uploading coverage, run some-lint-program on the code.
• Upload the lint results with harbormaster.sendmessage (in lint messages).

The harbormaster.sendmessage API page has detailed examples of how to format lint messages for the API call. arc itself also uses this API call in arc lint.