Inconclusive or false positive triage should re-enabled auto-approval

[wagnerand]

Describe the problem and steps to reproduce it:

When a version that is held for manual review gets triaged as inconclusive or false positive, we should re-enable auto-approval it.

What happened?

Clicking on inconclusive or false positive does not auto-approve the held version.

What did you expect to happen?

Clicking on inconclusive or false positive should auto-approve the held version.

Anything else we should know?

(Please include a link to the page, screenshots and any relevant files.)

[diox]

Because we only run actions once, it's too late to ignore the result in auto_approve. But we can implement something that reverts everything the actions do, that will set the auto_approval_delayed_until field to None, and the next tile auto_approve runs on that version it should be approved.

[willdurand]

We probably want to rename the feedback actions and/or add new action buttons because this is going to be confusing. Until now, these actions were about triaging the results with a focus on creating labelled data for the ML part.

By adding new behaviors to these feedback actions, names like "report as false positive" etc. don't make a lot of sense anymore.

We also want to add new behaviors such as banning a user or something in the near future, so while this issue can be fixed by a small patch before tomorrow's tag, I think we should step back and see how we want to improve @wagnerand's workflow (which goes beyond adding labels to scanner results).

[wagnerand]

I am also open to using a different approach that allows resuming auto-approval without having to approve the version using the reviewer tools, as that might be seen as an indicator that the version complies with the policies, which is not necessarily the case.

[diox]

I've got an implementation of this on a local branch, but I no longer think we should go with it in today's tag, because there are edge cases to consider.

One I've handled locally is if a version triggers 2 separate rules from 2 separate scanners, and one of the results is marked as false positive, then we need to not only revert the action from that result, but also make sure the actions for the other rule is still applied. One gotcha with this:

• If a version triggers 2 separate rules from the same scanner, then admins need to be very careful about marking as false positive: there is only one result, they should consider all matches for that result, and if only one of them is a false positive, then should not mark it as false positive, otherwise a malicious add-on might avoid the actions entirely.

Here are a couple other edge cases that are more complex and for which I don't have a good solution yet:

• If a version triggers a 24 hours delay action, then is marked by an admin as false positive during that 24 hours window, then reverted back to unknown, we have no choice but to revert the actions and then re-apply them, meaning the add-on would be delayed for 24 more hours... starting from the date of the revert.
• Worse, if a version triggers an infinite delay, and then another version is uploaded also triggering it, but the result for that second version is marked as false positive, then we are going to revert the action, causing the delay to be reset back to 0 (*) even though it should stay infinite.

(*) Actually, after discussion on slack we want to introduce a small 5 minutes to leave some room for error, but it amounts to the same thing: the infinite delay would be removed.

[wagnerand]

Re-prioritizing due to edge cases. I will file a new issue with limited scope in order to unblock versions being stuck.

[wagnerand]

Let's close this for now and evaluate holistically later. The triage was not designed for doing that, so apart from the edge-cases already mentioned, there might be further things to consider.