BAM Development (see page 25)

[Port]

The more reviewers you have, the bigger the chance that one is not doing his job correctly. While in theory this sounds like a great idea, it has the potential to have a lot of cracks in the system.

We're going to have very few reviewers that can actually approve or disapprove submissions.

[Ad Bot]

[Lugnut]

The more reviewers you have, the bigger the chance that one is not doing his job correctly. While in theory this sounds like a great idea, it has the potential to have a lot of cracks in the system.

yes, but someone else will catch it.
At the very least there is two people in the chain, a general reviewer and a privileged one.
at most, and hopefully, (but admittedly, lub is probably right) there will be around 4 in a chain.

There has to be more for larger mods.

[jes00]

We're going to be accepting a moderately large team of general reviewers - they can see pending submission/update "pull request" pages, comment on them, etc. as well as a small, very close team of privileged reviewers which are the only ones with the ability to actually approve or disapprove an add-on.

I don't see any point in having reviewers that can't review properly.

[Port]

I don't see any point in having reviewers that can't review properly.

The process of approving a submission (new add-on or add-on update) is less like a "here's this add-on and here's the buttons (but you can't use them because you're not privileged)" and more like a long, collaborative process extremely similar to the way that GitHub pull requests function, such as https://github.com/libgit2/libgit2/pull/2081. Note the importance of discussion in this context.

[Pecon]

I don't see any point in having reviewers that can't review properly.

Take another look at it. The idea of the general reviewers is to lighten the job of the privileged reviewers. I'd like to consider myself a generally good coder, but I know that I'm not experienced enough to understand all possible exploits. As a general reviewer, I would be able to collaborate with probably at least one other general reviewer to review the code for possible issues and leave information for the privileged reviewer. The privileged reviewer, knowing that the code has at least been reviewed already, can then more quickly decide whether the mod is alright for being approved and move on to reviewing more mods, much more quickly than if they had to review everything by themselves. The idea is basically that the general reviewers are just a community of commentators who help the privileged reviewers more quickly do their job.

[Lugnut]

That's a perfect example, Port.

Take another look at it. The idea of the general reviewers is to lighten the job of the privileged reviewers. I'd like to consider myself a generally good coder, but I know that I'm not experienced enough to understand all possible exploits. As a general reviewer, I would be able to collaborate with probably at least one other general reviewer to review the code for possible issues and leave information for the privileged reviewer. The privileged reviewer, knowing that the code has at least been reviewed already, can then more quickly decide whether the mod is alright for being approved and move on to reviewing more mods, much more quickly than if they had to review everything by themselves. The idea is basically that the general reviewers are just a community of commentators who help the privileged reviewers more quickly do their job.

Worst case scenario, you would just sort of write down the flow of the add-on, what does what, how it does it, and call the privileged reviewers attention to anything that you either don't understand or looks fishy.
After reading the flow of the code in English, then going to look at it in torquescript, the privileged reviewer has a much easier job, but can still make an informed decision.

[Ipquarx]

I don't see any point in having reviewers that can't review properly.

If someone shows that they're incapable of pruning out bad add-ons repeatedly, they'll be removed. Simple as that.

In addition, any add-on that has even the potential of being malicious that is uploaded will give a notification to the higher level reviewers, and they can take control of it and approve/disapprove it if the lower level reviewers don't catch something by mistake.

The goal here is that we have safety in numbers.

[Lugnut]

Furthermore, I'm thinking we'll all be paranoid as forget. This will increase the workload, but the goal is to not let a single malicious mod through.

In addition, any add-on that has even the potential of being malicious ...

I want to stress that. That's what the flagging thing is for. Mod reviewers should be able to put in flags of their own.
Another thing that wasn't really discussed in the topic is add-on updates - they'll use a diff system like git to show only the changes to the code. Ideally, we can have this in a web interface.
For a script only add-on with a measly 2 line update, a mod reviewer could have it checked out and approved in minutes, not hours or days.
Seriously - assuming notifications are working correctly, and optimal conditions,
add-on dev submits update for 2 lines of code > standard automated processing > lower level reviewer receives email notification on mobile device, checks everything out in a web interface, okays it > higher level reviewer gets notification and notices it's only 2 lines and they can probably handle the review while walking between classes or going to the bathroom or something > everything is okayed, theoretically in under 5 minutes.

Admittedly, there's all kinds of gaps in that, but for that little 2 line update I would say the absolute latest it could get approved is 24 hours after submission, which really isn't that long, and is the absolute upper end.

[Ipquarx]

The diff system needs to retain context though, so you can't have an addon with non-malicious code turning into a malicious one by adding a seemingly harmless line through an update.

So it would just say what files/lines were changed/added, so the reviewers can know what to look for, and anything with say... 30% or more of the the collective file space changed would have to go through a total review.

[jes00]

Regular reviewers should be able to approve/reject mods that are <= 50 lines and they should be able to approve/reject updates that contain <= 25 lines that were changed/added.

[Pecon]

Regular reviewers should be able to approve updates to mods if they're <= 25 lines that were modified.

Shouldn't be based on lines. Should be only if the changes have a very low 'threat level'.

[jes00]

Shouldn't be based on lines. Should be only if the changes have a very low 'threat level'.

Or both.

[Lugnut]

threat level could be determined using lines as a metric

also i'm not really comfortable with that. maybe it could be 'fast tracked' or something.

[Pecon]

Maybe have a middle-level type of reviewer? One that has the ability to approve but only for mods/updates that are very low threat level.

[Ipquarx]

threat level could be determined using lines as a metric

function wordsFromBeginning(%txt, %n)
{
   return getWords(%txt, 0, %n);
}

$text = "Hello world, how are you?";

for(%a = 5; %a >= 0; %a -= 2)
   echo(wordsFromBeginning($text, %a) SPC wordsFromBeginning($text, %a - 1));

7 lines, go ahead and try it out