Address PR comments

* Test coverage * Docs * Raw results Signed-off-by: Raghav Kaul <[email protected]>
ossf · Dec 17, 2022 · 44d0e02 · 44d0e02
1 parent 3e91332
commit 44d0e02
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 2 deletions.
diff --git a/checks/evaluation/code_review_test.go b/checks/evaluation/code_review_test.go
@@ -63,6 +63,22 @@ func TestCodeReview(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "Unreviewed human and bot changes",
+			expected: scut.TestReturn{
+				Score: checker.MinResultScore,
+			},
+			rawData: &checker.CodeReviewData{
+				DefaultBranchChangesets: []checker.Changeset{
+					{
+						Commits: []clients.Commit{{SHA: "a", Committer: clients.User{IsBot: true}}},
+					},
+					{
+						Commits: []clients.Commit{{SHA: "b"}},
+					},
+				},
+			},
+		},
 		{
 			name: "all human changesets reviewed, missing review on bot changeset",
 			expected: scut.TestReturn{

diff --git a/clients/githubrepo/graphql.go b/clients/githubrepo/graphql.go
@@ -397,6 +397,8 @@ func commitsFrom(data *graphqlData, repoOwner, repoName string) ([]clients.Commi
 				string(pr.Repository.Name) != repoName {
 				continue
 			}
+			// ResourcePath: e.g., for dependabot, "/apps/dependabot", or "/apps/renovate"
+			// Path that can be appended to "https://github.com" for a Github resource
 			openedByBot := strings.HasPrefix(string(pr.Author.ResourcePath), "/apps/")
 			associatedPR = clients.PullRequest{
 				Number:   int(pr.Number),

diff --git a/docs/checks.md b/docs/checks.md
@@ -207,14 +207,20 @@ malicious code (either as a malicious contributor or as an attacker who has
 subverted a contributor's account), because a reviewer might detect the
 subversion.
 
-The check determines whether the most recent (~30) commits have a Github-approved review
+The check determines whether the most recent changes (over the last ~30 commits) have 
+an approval on GitHub
 or if the merger is different from the committer (implicit review). It also
 performs a similar check for reviews using
 [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels
 "lgtm" or "approved") and [Gerrit](https://www.gerritcodereview.com/) ("Reviewed-on" and "Reviewed-by").
 If recent changes are solely bot activity (e.g. dependabot, renovatebot, or custom bots),
 the check returns inconclusively.
 
+Scoring is leveled instead of proportional to make the check more predictable.
+If any bot-originated changes are unreviewed, 3 points are deducted. If any human
+changes are unreviewed, 7 points are deducted if a single change is unreviewed, and
+another 3 are deducted if multiple changes are unreviewed.
+
 Note: Requiring reviews for all changes is infeasible for some projects, such as
 those with only one active participant. Even a project with multiple active
 contributors may not have enough active participation to be able to require

diff --git a/docs/checks/internal/checks.yaml b/docs/checks/internal/checks.yaml
@@ -307,14 +307,20 @@ checks:
       subverted a contributor's account), because a reviewer might detect the
       subversion.
 
-      The check determines whether the most recent (~30) commits have a Github-approved review
+      The check determines whether the most recent changes (over the last ~30 commits) have 
+      an approval on GitHub
       or if the merger is different from the committer (implicit review). It also
       performs a similar check for reviews using
       [Prow](https://github.com/kubernetes/test-infra/tree/master/prow#readme) (labels
       "lgtm" or "approved") and [Gerrit](https://www.gerritcodereview.com/) ("Reviewed-on" and "Reviewed-by").
       If recent changes are solely bot activity (e.g. dependabot, renovatebot, or custom bots),
       the check returns inconclusively.
 
+      Scoring is leveled instead of proportional to make the check more predictable.
+      If any bot-originated changes are unreviewed, 3 points are deducted. If any human
+      changes are unreviewed, 7 points are deducted if a single change is unreviewed, and
+      another 3 are deducted if multiple changes are unreviewed.
+
       Note: Requiring reviews for all changes is infeasible for some projects, such as
       those with only one active participant. Even a project with multiple active
       contributors may not have enough active participation to be able to require

diff --git a/pkg/json_raw_results.go b/pkg/json_raw_results.go
@@ -86,6 +86,7 @@ type jsonUser struct {
 	// Companies refer to a claim by a user in their profile.
 	Companies        []jsonCompany `json:"company,omitempty"`
 	NumContributions int           `json:"NumContributions,omitempty"`
+	IsBot            bool          `json:"isBot"`
 }
 
 type jsonContributors struct {
@@ -555,6 +556,7 @@ func (r *jsonScorecardRawResult) setDefaultCommitData(changesets []checker.Chang
 				State: r.State,
 				Reviewer: jsonUser{
 					Login: r.Author.Login,
+					IsBot: r.Author.IsBot,
 				},
 			})
 		}