✨ Feature: probe: unique code-reviewers

[andrelmbackman]

Is your feature request related to a problem? Please describe.
The Code-review check currently checks if there are unreviewed changesets and only counts reviewers whose logins != author login. If the logins can't be retrieved (ex. they were reviewed outside of GitHub) the check gets full points. This is an understandable approach until the scorecard can handle other review platforms, but not the safest.

Describe the solution you'd like
I am working on a probe that checks that:
A. the logins of both author and reviewers are retrieved(otherwise the results are inconclusive)
B. that there are at least two unique reviewers per changeset.
The amount of minimum unique reviewers wanted for a positive outcome can easily be changed in the impl.go file.

Describe alternatives you've considered
To update the scorecard's retrieval of raw data to support multiple review platforms, this would also be needed to utilize the probe's full potential. This, however, is another issue for another day. Not all users of the scorecard is interested in an aggregate score, but want more binary conditions that either pass or fail, hence the probe.

[raghavkaul]

Hi, I think having something like these three probes makes sense:

1. codeReviewed - All changesets have some review activity
2. codeApproved - All changesets are approved by at least one actor who is not the original PR author (this would be conclusive even if fetching/validating the reviewer identity fails)
3. codeReviewedTwoReviewers - your proposed criteria could fall here - All changesets are approved by atleast two actors, and A. the login of the author and reviewer are distinct, and B. there are two distinct reviewers

The logic for checking author login != reviewer login could look something like this:

scorecard/checks/evaluation/code_review.go

Line 95 in e1eede2

if review.State == "APPROVED" && review.Author.Login != changeset.Author.Login {

Currently, for platforms outside of GitHub, we give full credit/points for Code Review, even though we just check the git commit message (which can be tampered with). In the future, we could invoke the Phabricator API to check whether the differential revision 1) exists and 2) was reviewed by the author listed in the commit message. We would need to rethink the logic here:

scorecard/checks/evaluation/code_review.go

Line 87 in e1eede2

if plat != checker.ReviewPlatformUnknown &&

Scorecard has always trusted GitHub and the commit log, and doesn't go to huge lengths to validate the integrity of, say, committer identities. It makes sense to add structured results though, since changing Code-Review evaluation for existing repos that live on GitHub but don't use GitHub for code review would regress scoring somewhat confusingly.

[andrelmbackman]

Thank you for your thorough input! I have the last probe you mentioned working, but I will work on the first and second as well.
Could you clarify what you mean by review activity, what does it look like in the raw results?

[laurentsimon]

Could you clarify what you mean by review activity, what does it look like in the raw results?

same question. I potentially see:

• Reviewed. There is at least one review with PR author != reviewer - this is implemented
• TwoReviewed. There is at least 2 reviews with PR author != reviewer - this is implemented
• ReviewedNoCommit. Reviewer did not commit code to the PR author's branch - this is not implemented

[github-actions]

Stale issue message - this issue will be closed in 7 days

[github-actions]

This issue is stale because it has been open for 60 days with no activity.