✨ show non-compliant code changes for CI-Tests, Code-Review and SAST checks in --show-details mode

checks/evaluation/ci_tests.go

@@ -62,7 +62,7 @@ func CITests(name string, c *checker.CITestData, dl checker.DetailLogger) checke
 			// Log message says commit, but really we only care about PRs, and
 			// use only one commit (branch HEAD) to refer to all commits in a PR
 			dl.Debug(&checker.LogMessage{
-				Text: fmt.Sprintf("merged PR without CI test at HEAD: %s", r.HeadSHA),
+				Text: fmt.Sprintf("merged PR %d without CI test at HEAD: %s", r.PullRequestNumber, r.HeadSHA),
 			})
 		}
 	}

checks/evaluation/code_review.go

@@ -16,6 +16,7 @@ package evaluation
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/ossf/scorecard/v4/checker"
 	sce "github.com/ossf/scorecard/v4/errors"
@@ -46,6 +47,8 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
 	foundBotReviewActivity := false
 	nUnreviewedHumanChanges := 0
 	nUnreviewedBotChanges := 0
+
+	var unreviewedHumanRevisionIDs, unreviewedBotRevisionIDs []string
 	for i := range r.DefaultBranchChangesets {
 		cs := &r.DefaultBranchChangesets[i]
 		isReviewed := reviewScoreForChangeset(cs) >= changesReviewed
@@ -58,11 +61,26 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
 			foundHumanReviewActivity = true
 		case !isReviewed && isBotCommit:
 			nUnreviewedBotChanges += 1
+			unreviewedBotRevisionIDs = append(unreviewedBotRevisionIDs, cs.RevisionID)
 		case !isReviewed && !isBotCommit:
 			nUnreviewedHumanChanges += 1
+			unreviewedHumanRevisionIDs = append(unreviewedHumanRevisionIDs, cs.RevisionID)
 		}
 	}
 
+	// Let's include non-compliant revision IDs in details
+	if len(unreviewedHumanRevisionIDs) > 0 {
+		dl.Debug(&checker.LogMessage{
+			Text: fmt.Sprintf("List of revision IDs not reviewed by humans:%s", strings.Join(unreviewedHumanRevisionIDs, ",")),
+		})
+	}
+
+	if len(unreviewedBotRevisionIDs) > 0 {
+		dl.Debug(&checker.LogMessage{
+			Text: fmt.Sprintf("List of bot revision IDs not reviewed by humans:%s", strings.Join(unreviewedBotRevisionIDs, ",")),
+		})
+	}
+
 	if foundBotReviewActivity && !foundHumanReviewActivity {
 		reason := fmt.Sprintf("found no human review activity in the last %v changesets", len(r.DefaultBranchChangesets))
 		return checker.CreateInconclusiveResult(name, reason)

checks/evaluation/code_review_test.go

@@ -50,7 +50,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "NoReviews",
 			expected: scut.TestReturn{
-				Score: checker.MinResultScore,
+				Score:         checker.MinResultScore,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
@@ -66,7 +67,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "Unreviewed human and bot changes",
 			expected: scut.TestReturn{
-				Score: checker.MinResultScore,
+				Score:         checker.MinResultScore,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
@@ -82,7 +84,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "all human changesets reviewed, missing review on bot changeset",
 			expected: scut.TestReturn{
-				Score: 7,
+				Score:         7,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{
@@ -147,7 +150,8 @@ func TestCodeReview(t *testing.T) {
 		{
 			name: "bot commits only",
 			expected: scut.TestReturn{
-				Score: checker.InconclusiveResultScore,
+				Score:         checker.InconclusiveResultScore,
+				NumberOfDebug: 1,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{

checks/sast.go

@@ -50,7 +50,7 @@ func init() {
 
 // SAST runs SAST check.
 func SAST(c *checker.CheckRequest) checker.CheckResult {
-	sastScore, sastErr := sastToolInCheckRuns(c)
+	sastScore, nonCompliantPRs, sastErr := sastToolInCheckRuns(c)
 	if sastErr != nil {
 		return checker.CreateRuntimeErrorResult(CheckSAST, sastErr)
 	}
@@ -96,6 +96,9 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 		case codeQlScore == checker.MaxResultScore:
 			const sastWeight = 3
 			const codeQlWeight = 7
+			c.Dlogger.Debug(&checker.LogMessage{
+				Text: getNonCompliantPRMessage(nonCompliantPRs),
+			})
 			score := checker.AggregateScoresWithWeight(map[int]int{sastScore: sastWeight, codeQlScore: codeQlWeight})
 			return checker.CreateResultWithScore(CheckSAST, "SAST tool detected but not run on all commmits", score)
 		default:
@@ -117,6 +120,9 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 			return checker.CreateMaxScoreResult(CheckSAST, "SAST tool is run on all commits")
 		}
 
+		c.Dlogger.Debug(&checker.LogMessage{
+			Text: getNonCompliantPRMessage(nonCompliantPRs),
+		})
 		return checker.CreateResultWithScore(CheckSAST,
 			checker.NormalizeReason("SAST tool is not run on all commits", sastScore), sastScore)
 	}
@@ -125,15 +131,16 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 	return checker.CreateRuntimeErrorResult(CheckSAST, sce.WithMessage(sce.ErrScorecardInternal, "contact team"))
 }
 
-func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
+func sastToolInCheckRuns(c *checker.CheckRequest) (int, map[int]int, error) {
 	commits, err := c.RepoClient.ListCommits()
 	if err != nil {
-		return checker.InconclusiveResultScore,
+		return checker.InconclusiveResultScore, nil,
 			sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("RepoClient.ListCommits: %v", err))
 	}
 
 	totalMerged := 0
 	totalTested := 0
+	nonCompliantPRs := make(map[int]int)
 	for i := range commits {
 		pr := commits[i].AssociatedMergeRequest
 		// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
@@ -142,9 +149,10 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 			continue
 		}
 		totalMerged++
+		checked := false
 		crs, err := c.RepoClient.ListCheckRunsForRef(pr.HeadSHA)
 		if err != nil {
-			return checker.InconclusiveResultScore,
+			return checker.InconclusiveResultScore, nil,
 				sce.WithMessage(sce.ErrScorecardInternal, fmt.Sprintf("Client.Checks.ListCheckRunsForRef: %v", err))
 		}
 		// Note: crs may be `nil`: in this case
@@ -163,15 +171,19 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 					Text: fmt.Sprintf("tool detected: %v", cr.App.Slug),
 				})
 				totalTested++
+				checked = true
 				break
 			}
 		}
+		if !checked {
+			nonCompliantPRs[pr.Number] = pr.Number
+		}
 	}
 	if totalMerged == 0 {
 		c.Dlogger.Warn(&checker.LogMessage{
 			Text: "no pull requests merged into dev branch",
 		})
-		return checker.InconclusiveResultScore, nil
+		return checker.InconclusiveResultScore, nil, nil
 	}
 
 	if totalTested == totalMerged {
@@ -184,7 +196,7 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 		})
 	}
 
-	return checker.CreateProportionalScore(totalTested, totalMerged), nil
+	return checker.CreateProportionalScore(totalTested, totalMerged), nonCompliantPRs, nil
 }
 
 func codeQLInCheckDefinitions(c *checker.CheckRequest) (int, error) {
@@ -366,3 +378,14 @@ func findLine(content, data []byte) (uint, error) {
 
 	return 0, nil
 }
+
+func getNonCompliantPRMessage(intMap map[int]int) string {
+	var sb strings.Builder
+	for _, value := range intMap {
+		if len(sb.String()) != 0 {
+			sb.WriteString(", ")
+		}
+		sb.WriteString(fmt.Sprintf("%d", value))
+	}
+	return fmt.Sprintf("List of pull requests without CI test: %s", sb.String())
+}

e2e/code_review_test.go

@@ -50,7 +50,7 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 				Score:         checker.MinResultScore,
 				NumberOfWarn:  0,
 				NumberOfInfo:  0,
-				NumberOfDebug: 0,
+				NumberOfDebug: 1,
 			}
 			result := checks.CodeReview(&req)
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
@@ -75,7 +75,7 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 				Score:         checker.MinResultScore,
 				NumberOfWarn:  0,
 				NumberOfInfo:  0,
-				NumberOfDebug: 0,
+				NumberOfDebug: 1,
 			}
 			result := checks.CodeReview(&req)
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
@@ -117,7 +117,8 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 				Dlogger:    &dl,
 			}
 			expected := scut.TestReturn{
-				Score: checker.InconclusiveResultScore,
+				Score:         checker.InconclusiveResultScore,
+				NumberOfDebug: 1,
 			}
 			result := checks.CodeReview(&req)
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())
@@ -138,7 +139,8 @@ var _ = Describe("E2E TEST:"+checks.CheckCodeReview, func() {
 				Dlogger:    &dl,
 			}
 			expected := scut.TestReturn{
-				Score: checker.MinResultScore,
+				Score:         checker.MinResultScore,
+				NumberOfDebug: 1,
 			}
 			result := checks.CodeReview(&req)
 			Expect(scut.ValidateTestReturn(nil, "use code reviews", &expected, &result, &dl)).Should(BeTrue())