draft

checker/raw_result.go

@@ -28,7 +28,7 @@ type RawResults struct {
 // CodeReviewData contains the raw results
 // for the Code-Review check.
 type CodeReviewData struct {
-	Commits []Commit
+	DefaultBranchCommits []DefaultBranchCommit
 }
 
 // VulnerabilitiesData contains the raw results
@@ -111,21 +111,28 @@ type Issue struct {
 	// TODO: add fields, e.g., state=[opened|closed]
 }
 
-// Commit represents a commit.
-type Commit struct {
-	// Note: not all commits are reviewed.
-	// `nil` indicate no reviews.
-	Review *Review
-	// Note: SHA is not directly accessible from a pull request.
-	// TODO:SHA          string
-	Committer User
+// DefaultBranchCommit represents a commit
+// to the default branch.
+type DefaultBranchCommit struct {
+	// Review indicate whether the commit was reviewed,
+	// using scorecard's logic.
+	// Note: this field may disppear if we decide the purely "raw"
+	// results are enough.
+	ApprovedReviews *ApprovedReviews
+	// Fields below are taken directly from cloud
+	// version control systems, e.g. GitHub.
+	SHA           string
+	Committer     User
+	CommitMessage string
+	MergeRequest  *MergeRequest
 }
 
 // MergeRequest represents a merge request.
 type MergeRequest struct {
-	Author User
-	Number int
-	// TODO: add fields, e.g., State=["merged"|"closed"]
+	Author  User
+	Number  int
+	Labels  []string
+	Reviews []Review
 }
 
 // User represent a user.
@@ -140,6 +147,8 @@ var (
 	ReviewPlatformProw = "Prow"
 	// ReviewPlatformGerrit is the name of ReviewPlatform for Gerrit.
 	ReviewPlatformGerrit = "Gerrit"
+	// ReviewStateApproved means an approved review.
+	ReviewStateApproved = "approved"
 )
 
 // ReviewPlatform represents a review platform.
@@ -148,13 +157,19 @@ type ReviewPlatform struct {
 	// TODO: add fields, e.g. config files, etc.
 }
 
-// Review represents a review.
+// ApprovedReviews represents the LGTMs associated with a commit
+// to the default branch.
+type ApprovedReviews struct {
+	Platform ReviewPlatform
+	// Note: this field is only populated for GitHub and Prow.
+	MaintainerReviews []Review
+}
+
+// Review represent a single-maintainer's review.
 type Review struct {
-	// Note: Not all reviews contain a merge request, e.g. for Gerrit.
-	// `nil` indicates there are no merge requests associated with a commit.
-	MergeRequest *MergeRequest
-	Platform     ReviewPlatform
-	Authors      []User
+	Reviewer User
+	State    string
+	// TODO(Review): add fields here if needed.
 }
 
 // File represents a file.

checks/evaluation/code_review.go

@@ -38,8 +38,8 @@ func CodeReview(name string, dl checker.DetailLogger,
 		checker.ReviewPlatformGerrit: 0,
 	}
 
-	for _, commit := range r.Commits {
-		if commit.Review == nil && isBot(commit.Committer.Login) {
+	for _, commit := range r.DefaultBranchCommits {
+		if commit.ApprovedReviews == nil && isBot(commit.Committer.Login) {
 			dl.Debug3(&checker.LogMessage{
 				Text: fmt.Sprintf("skip commit from bot account: %s", commit.Committer),
 			})
@@ -50,25 +50,25 @@ func CodeReview(name string, dl checker.DetailLogger,
 		totalCommits++
 
 		// No reviews.
-		if commit.Review == nil {
+		if commit.ApprovedReviews == nil {
 			continue
 		}
 
-		totalReviewed[commit.Review.Platform.Name]++
+		totalReviewed[commit.ApprovedReviews.Platform.Name]++
 
-		switch commit.Review.Platform.Name {
+		switch commit.ApprovedReviews.Platform.Name {
 		// GitHub reviews.
 		case checker.ReviewPlatformGitHub:
 			dl.Debug3(&checker.LogMessage{
 				Text: fmt.Sprintf("%s #%d merge request approved",
-					checker.ReviewPlatformGitHub, commit.Review.MergeRequest.Number),
+					checker.ReviewPlatformGitHub, commit.MergeRequest.Number),
 			})
 
 		// Prow reviews.
 		case checker.ReviewPlatformProw:
 			dl.Debug3(&checker.LogMessage{
 				Text: fmt.Sprintf("%s #%d merge request approved",
-					checker.ReviewPlatformProw, commit.Review.MergeRequest.Number),
+					checker.ReviewPlatformProw, commit.MergeRequest.Number),
 			})
 
 		// Gerrit reviews.

checks/raw/code_review.go

@@ -20,13 +20,29 @@ import (
 
 	"github.com/ossf/scorecard/v4/checker"
 	"github.com/ossf/scorecard/v4/clients"
+	sce "github.com/ossf/scorecard/v4/errors"
 )
 
 // CodeReview retrieves the raw data for the Code-Review check.
 func CodeReview(c clients.RepoClient) (checker.CodeReviewData, error) {
-	results := []checker.Commit{}
+	results := []checker.DefaultBranchCommit{}
 
-	// 1. Look at merge requests.
+	// 1. Look at the latest commits
+	commits, err := c.ListCommits()
+	if err != nil {
+		return checker.CodeReviewData{}, fmt.Errorf("%w", err)
+	}
+
+	oc := make(map[string]checker.DefaultBranchCommit)
+	for _, commit := range commits {
+		com := commitRequest(commit)
+		results = append(results, com)
+		// Keep an index of commits by SHA.
+		oc[commit.SHA] = com
+		fmt.Println("adding", commit.SHA, commit.Committer.Login, com.Committer.Login)
+	}
+
+	// 2. Look at merge requests.
 	mrs, err := c.ListMergedPRs()
 	if err != nil {
 		return checker.CodeReviewData{}, fmt.Errorf("%w", err)
@@ -38,87 +54,117 @@ func CodeReview(c clients.RepoClient) (checker.CodeReviewData, error) {
 			continue
 		}
 
-		// We have a merge request.
-		com := checker.Commit{
-			Committer: checker.User{
-				Login: mr.MergeCommit.Committer.Login,
-			},
-			Review: reviewData(&mr),
+		// If the merge request is not a recent commit, skip.
+		com, exists := oc[mr.MergeCommit.SHA]
+		if !exists {
+			fmt.Println("skipping", mr.MergeCommit.SHA, mr.Number)
+			continue
 		}
-		results = append(results, com)
-	}
 
-	if len(results) > 0 {
-		return checker.CodeReviewData{Commits: results}, nil
-	}
+		// Sanity checks the logins are the same.
+		if com.Committer.Login != mr.MergeCommit.Committer.Login {
+			fmt.Println(mr.MergeCommit.SHA, mr.Number, com.Committer.Login, mr.MergeCommit.Committer.Login)
+			return checker.CodeReviewData{}, sce.WithMessage(sce.ErrScorecardInternal,
+				fmt.Sprintf("commit login (%s) different from merge request commit login (%s)",
+					com.Committer.Login, mr.MergeCommit.Committer.Login))
+		}
 
-	// 2. Look at commits.
-	commits, err := c.ListCommits()
-	if err != nil {
-		return checker.CodeReviewData{}, fmt.Errorf("%w", err)
-	}
+		// We have a recent merge request: add other fields.
+		com.ApprovedReviews = approvedReviews(&mr, &com)
+		com.MergeRequest = mergeRequest(&mr)
 
-	for _, commit := range commits {
-		com := commitRequestData(commit)
 		results = append(results, com)
 	}
 
-	return checker.CodeReviewData{Commits: results}, nil
+	if len(results) > 0 {
+		return checker.CodeReviewData{DefaultBranchCommits: results}, nil
+	}
+
+	return checker.CodeReviewData{DefaultBranchCommits: results}, nil
 }
 
-func reviewPlatform(platform string) checker.Review {
-	review := checker.Review{
+func reviewPlatform(platform string) checker.ApprovedReviews {
+	mr := checker.ApprovedReviews{
 		Platform: checker.ReviewPlatform{
 			Name: platform,
 		},
 	}
-	return review
+	return mr
 }
 
-func commitRequestData(c clients.Commit) checker.Commit {
-	r := checker.Commit{
+func commitRequest(c clients.Commit) checker.DefaultBranchCommit {
+	r := checker.DefaultBranchCommit{
 		Committer: checker.User{
 			Login: c.Committer.Login,
 		},
+		SHA:           c.SHA,
+		CommitMessage: c.Message,
 	}
 
-	if isReviewedOnGerrit(c) {
-		review := reviewPlatform(checker.ReviewPlatformGerrit)
-		r.Review = &review
+	return r
+}
+
+func mergeRequest(mr *clients.PullRequest) *checker.MergeRequest {
+	r := checker.MergeRequest{
+		Number: mr.Number,
+		Author: checker.User{
+			Login: mr.Author.Login,
+		},
+
+		Labels:  labels(mr),
+		Reviews: reviews(mr),
 	}
+	return &r
+}
 
-	return r
+func labels(mr *clients.PullRequest) []string {
+	labels := []string{}
+	for _, l := range mr.Labels {
+		labels = append(labels, l.Name)
+	}
+	return labels
 }
 
-func reviewData(mr *clients.PullRequest) *checker.Review {
-	var review checker.Review
+func approvedReviews(mr *clients.PullRequest, c *checker.DefaultBranchCommit) *checker.ApprovedReviews {
+	var review checker.ApprovedReviews
 
 	// Review platform.
-	// Note: Gerrit does not use merge requests and is checked
-	// via commits in commitRequestData().
 	switch {
 	case isReviewedOnGitHub(mr):
 		review = reviewPlatform(checker.ReviewPlatformGitHub)
-		review.Authors = reviewAuthors(mr)
+		review.MaintainerReviews = maintainerReviews(mr)
 
 	case isReviewedOnProw(mr):
 		review = reviewPlatform(checker.ReviewPlatformProw)
-	}
 
-	// Add merge request.
-	r := checker.MergeRequest{
-		Number: mr.Number,
-		Author: checker.User{
-			Login: mr.Author.Login,
-		},
+	case isReviewedOnGerrit(c):
+		review = reviewPlatform(checker.ReviewPlatformGerrit)
+
 	}
-	review.MergeRequest = &r
 
 	return &review
 }
 
-func reviewAuthors(mr *clients.PullRequest) []checker.User {
-	authors := []checker.User{}
+func reviews(mr *clients.PullRequest) []checker.Review {
+	reviews := []checker.Review{}
+	for _, m := range mr.Reviews {
+		r := checker.Review{
+			State: m.State,
+		}
+
+		if m.Author != nil &&
+			m.Author.Login != "" {
+			r.Reviewer = checker.User{
+				Login: m.Author.Login,
+			}
+		}
+		reviews = append(reviews, r)
+	}
+	return reviews
+}
+
+func maintainerReviews(mr *clients.PullRequest) []checker.Review {
+	reviews := []checker.Review{}
 	mauthors := make(map[string]bool)
 	for _, m := range mr.Reviews {
 		if !(m.State == "APPROVED" &&
@@ -128,30 +174,35 @@ func reviewAuthors(mr *clients.PullRequest) []checker.User {
 		}
 
 		if _, exists := mauthors[m.Author.Login]; !exists {
-			authors = append(authors, checker.User{
-				Login: m.Author.Login,
+			reviews = append(reviews, checker.Review{
+				Reviewer: checker.User{
+					Login: m.Author.Login,
+				},
+				State: checker.ReviewStateApproved,
 			})
 			// Needed because it's is possible for a user
 			// to approve a merge request multiple times.
 			mauthors[m.Author.Login] = true
 		}
 	}
 
-	if len(authors) > 0 {
-		return authors
+	if len(reviews) > 0 {
+		return reviews
 	}
 
 	// Check if the merge request is committed by someone other than author. This is kind
 	// of equivalent to a review and is done several times on small prs to save
 	// time on clicking the approve button.
 	if mr.MergeCommit.Committer.Login != "" &&
 		mr.MergeCommit.Committer.Login != mr.Author.Login {
-		authors = append(authors, checker.User{
-			Login: mr.MergeCommit.Committer.Login,
+		reviews = append(reviews, checker.Review{
+			State: checker.ReviewStateApproved,
+			Reviewer: checker.User{
+				Login: mr.MergeCommit.Committer.Login,
+			},
 		})
 	}
-
-	return authors
+	return reviews
 }
 
 func isReviewedOnGitHub(mr *clients.PullRequest) bool {
@@ -181,8 +232,8 @@ func isReviewedOnProw(mr *clients.PullRequest) bool {
 	return false
 }
 
-func isReviewedOnGerrit(c clients.Commit) bool {
-	commitMessage := c.Message
-	return strings.Contains(commitMessage, "\nReviewed-on: ") &&
-		strings.Contains(commitMessage, "\nReviewed-by: ")
+func isReviewedOnGerrit(c *checker.DefaultBranchCommit) bool {
+	m := c.CommitMessage
+	return strings.Contains(m, "\nReviewed-on: ") &&
+		strings.Contains(m, "\nReviewed-by: ")
 }

clients/githubrepo/graphql.go

@@ -70,6 +70,7 @@ type graphqlData struct {
 							Login githubv4.String
 						}
 					}
+					Oid githubv4.GitObjectID
 				}
 				MergedAt githubv4.DateTime
 				Labels   struct {
@@ -193,6 +194,7 @@ func pullRequestsFrom(data *graphqlData) []clients.PullRequest {
 				Committer: clients.User{
 					Login: string(pr.MergeCommit.Author.User.Login),
 				},
+				SHA: string(pr.MergeCommit.Oid),
 			},
 		}
 		for _, label := range pr.Labels.Nodes {