✨ 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 botReviewActivityRevisionIDs, unreviewedHumanRevisionIDs, unreviewedBotRevisionIDs []string
 	for i := range r.DefaultBranchChangesets {
 		cs := &r.DefaultBranchChangesets[i]
 		isReviewed := reviewScoreForChangeset(cs) >= changesReviewed
@@ -54,15 +57,37 @@ func CodeReview(name string, dl checker.DetailLogger, r *checker.CodeReviewData)
 		switch {
 		case isReviewed && isBotCommit:
 			foundBotReviewActivity = true
+			botReviewActivityRevisionIDs = append(botReviewActivityRevisionIDs, cs.RevisionID)
 		case isReviewed && !isBotCommit:
 			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(botReviewActivityRevisionIDs) > 0 {
+		dl.Debug(&checker.LogMessage{
+			Text: fmt.Sprintf("List of revision IDs by bots:%s", strings.Join(botReviewActivityRevisionIDs, ",")),
+		})
+	}
+
+	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: 2,
 			},
 			rawData: &checker.CodeReviewData{
 				DefaultBranchChangesets: []checker.Changeset{

checks/sast.go

@@ -49,7 +49,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)
 	}
@@ -95,6 +95,9 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 		case codeQlScore == checker.MaxResultScore:
 			const sastWeight = 3
 			const codeQlWeight = 7
+			c.Dlogger.Warn(&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:
@@ -116,6 +119,9 @@ func SAST(c *checker.CheckRequest) checker.CheckResult {
 			return checker.CreateMaxScoreResult(CheckSAST, "SAST tool is run on all commits")
 		}
 
+		c.Dlogger.Warn(&checker.LogMessage{
+			Text: getNonCompliantPRMessage(nonCompliantPRs),
+		})
 		return checker.CreateResultWithScore(CheckSAST,
 			checker.NormalizeReason("SAST tool is not run on all commits", sastScore), sastScore)
 	}
@@ -124,15 +130,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, []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 := []int{}
 	for i := range commits {
 		pr := commits[i].AssociatedMergeRequest
 		// TODO(#575): We ignore associated PRs if Scorecard is being run on a fork
@@ -141,9 +148,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
@@ -162,15 +170,19 @@ func sastToolInCheckRuns(c *checker.CheckRequest) (int, error) {
 					Text: fmt.Sprintf("tool detected: %v", cr.App.Slug),
 				})
 				totalTested++
+				checked = true
 				break
 			}
 		}
+		if !checked {
+			nonCompliantPRs = append(nonCompliantPRs, 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 {
@@ -183,7 +195,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) {
@@ -323,3 +335,14 @@ func findLine(content, data []byte) (uint, error) {
 
 	return 0, nil
 }
+
+func getNonCompliantPRMessage(intSlice []int) string {
+	var sb strings.Builder
+	for i, value := range intSlice {
+		if i > 0 {
+			sb.WriteString(", ")
+		}
+		sb.WriteString(fmt.Sprintf("%d", value))
+	}
+	return fmt.Sprintf("List of pull requests without CI test: %s", sb.String())
+}