BUG: Code-review check result is wrong

[bhuvi11]

Hello Team,

I see that the code review check is not providing the right result,

{
"Details": [
"found pr with committer different than author: 1",
"found pr with committer different than author: 2",
"found pr with committer different than author: 9",
"found pr with committer different than author: 33",
"Gerrit code reviews found for 0 commits out of the last 30",
"Prow code reviews found for 0 commits out of the last 4"
],
"Score": 10,
"Reason": "GitHub code reviews found for 4 commits out of the last 4 -- score normalized to 10",
"Name": "Code-Review"
},

Here the pr number 33 is having bhuvi11 as the author and committer, but it is mentioned as different.
Why is it so?

PR for reference

https://github.com/bhuvi11/ecom-service/pull/33/commits

Thanks in advance

[azeemshaikh38]

The GitHub API returns the committer of the PR as null which leads to Scorecard recognizing author and committer as different. Giving a score of 10 here seems like the wrong behavior. @laurentsimon this is an interesting corner case to look into.

[laurentsimon]

@bhuvi11 thanks for the report!

Can you give me the steps to reproduce the problem (and how the PR was merged)? Example:

1. Create a branch on the same repo
2. XXX
3. etc

I need to figure out why the committer is null and whether we can tweak the message/score.

[bhuvi11]

Hello @laurentsimon,
Sure

• I have created a branch called testing
• I made a change
• I raised a PR and merged it with master

Pull request :

bhuvi11/ecom-service#40

Branch successfully merged

[laurentsimon]

Thanks for the info. I think @azeemsgoogle got it right: nil is returned because you reviewed your own PR which does not count as a review. The check is looking for evidence that someone else reviewed the code, in order to reduce the risk of insider attacks.

We should check for nil and treat it as being the same as the reviewer, which eventually will return a 0 score.

@bhuvi11 do you agree with this assessment? If you do, can you send us a PR to fix it? (should a single-line change).

Again, thank you for bringing this problem to our attention. Much appreciated!

[bhuvi11]

Hello @laurentsimon,
Thanks a lot for the response.
What is that i need to do?

[laurentsimon]

@azeemsgoogle recently updated this code with GraphQl. It may have fixed the problem. Can you run check if the changes have fixed the problem?

[bhuvi11]

Hello @laurentsimon,

I just ran and got this as my result
{
"details":[
"Info: Gerrit code reviews found for 0 commits out of the last 30",
"Info: Prow code reviews found for 0 commits out of the last 5"
],
"score":10,
"reason":"GitHub code reviews found for 5 commits out of the last 5 -- score normalized to 10",
"name":"Code-Review"
}
I believe i got score 10 because code review were done for all the commits? So it is a good case right ?

[laurentsimon]

I think that's incorrect, because you reviewed your own commits. @azeemsgoogle is there a way to fix this with codeQL? (the value seems to be returned by the server https://github.com/ossf/scorecard/blob/main/clients/githubrepo/graphql.go#L151)

[bhuvi11]

Hello @laurentsimon , So this issue is resolved?

[azeemshaikh38]

I don't think so. @laurentsimon could you please look into this?

./scorecard --repo=bhuvi11/ecom-service --checks=Code-Review
Starting [Code-Review]
Finished [Code-Review]

RESULTS
-------
Aggregate score: 10.0 / 10

Check scores:
|---------|-------------|--------------------------------|------------------------------------------------------------------------------------------------------------|
|  SCORE  |    NAME     |             REASON             |                                         DOCUMENTATION/REMEDIATION                                          |
|---------|-------------|--------------------------------|------------------------------------------------------------------------------------------------------------|
| 10 / 10 | Code-Review | GitHub code reviews found for  | https://github.com/ossf/scorecard/blob/aa93ac232925102ff71023081932f8630778d2bf/docs/checks.md#code-review |
|         |             | 5 commits out of the last 5 -- |                                                                                                            |
|         |             | score normalized to 10         |                                                                                                            |
|---------|-------------|--------------------------------|------------------------------------------------------------------------------------------------------------|