cmd/gerritbot: automatically flag common issues in PRs

In this CL, we add a set of checks that automatically run
against a new GitHub PR after it is imported into Gerrit
in order to flag common issues, such as missing a bug
reference or ending the first line of the commit with
a period. See below for the current set of defined rules.

The intent is to save maintainer time, as well as
perhaps make a better initial experience for a
new contributor via faster feedback with potentially
helpful linked resources.

By attempting to engage a new contributor as soon as possible
along with some pointers, we also hope to get them
to reply (successfully) in Gerrit, which in turn helps
a potential reviewer see that the contributor knows
how to use Gerrit.

The package comment in rules.go provides an overview
of the implementation approach.

The rules themselves are defined as short functions
in rules.go.

We currently have 100% test coverage within the rules package.

Sample results from running on a corpus of 1000 GitHub PRs
that were imported into Gerrit:

  Avg. findings per CL: 1.42
  CLs with findings:    74.5%

  CL hit %   Finding
  --------   ------------------------------------------------
      9.9%   title: no package found
      0.1%   title: no colon then single space after package
     13.0%   title: no lowercase word after a first colon
      2.0%   title: ends with period
     20.8%   body: short
      6.1%   body: no sentence candidates found
     21.7%   body: long lines
     10.5%   body: might use markdown
      2.2%   body: still contains PR instructions
      0.9%   body: contains Signed-off-by
     43.9%   body: no bug reference candidate found
      9.3%   body: bug format looks incorrect
      1.5%   body: no bug reference candidate at end

See golang/go#61573 for additional background.

Updates golang/go#61316
Fixes golang/go#61573

Change-Id: I866cf650608d6ce9c6783aabc17a219f1815908c
Reviewed-on: https://go-review.googlesource.com/c/build/+/513397
Auto-Submit: Dmitri Shuralyov <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Damien Neil <[email protected]>
Run-TryBot: t hepudds <[email protected]>
Reviewed-by: Heschi Kreinick <[email protected]>

Author: thepudds

cmd/gerritbot/gerritbot.go

@@ -28,6 +28,7 @@ import (
 	"cloud.google.com/go/compute/metadata"
 	"github.com/google/go-github/v48/github"
 	"github.com/gregjones/httpcache"
+	"golang.org/x/build/cmd/gerritbot/internal/rules"
 	"golang.org/x/build/gerrit"
 	"golang.org/x/build/internal/https"
 	"golang.org/x/build/internal/secret"
@@ -729,7 +730,8 @@ func (b *bot) importGerritChangeFromPR(ctx context.Context, pr *github.PullReque
 		pushOpts = "%ready"
 	}
 
-	if cl == nil {
+	newCL := cl == nil
+	if newCL {
 		// Add this informational message only on CL creation.
 		msg := fmt.Sprintf("This Gerrit CL corresponds to GitHub PR %s.\n\nAuthor: %s", prShortLink(pr), author)
 		pushOpts += ",m=" + url.QueryEscape(msg)
@@ -766,7 +768,57 @@ Please visit Gerrit at %s.
   * You should word wrap the PR description at ~76 characters unless you need longer lines (e.g., for tables or URLs).
 * See the [Sending a change via GitHub](https://go.dev/doc/contribute#sending_a_change_github) and [Reviews](https://go.dev/doc/contribute#reviews) sections of the Contribution Guide as well as the [FAQ](https://github.com/golang/go/wiki/GerritBot/#frequently-asked-questions) for details.`,
 		pr.Head.GetSHA(), changeURL)
-	return b.postGitHubMessageNoDup(ctx, repo.GetOwner().GetLogin(), repo.GetName(), pr.GetNumber(), "", msg)
+	err = b.postGitHubMessageNoDup(ctx, repo.GetOwner().GetLogin(), repo.GetName(), pr.GetNumber(), "", msg)
+	if err != nil {
+		return err
+	}
+
+	if newCL {
+		// Check if we spot any problems with the CL according to our internal
+		// set of rules, and if so, add an unresolved comment to Gerrit.
+		// If the author responds to this, it also helps a reviewer see the author has
+		// registered for a Gerrit account and knows how to reply in Gerrit.
+		// TODO: see CL 509135 for possible follow-ups, including possibly always
+		// asking explicitly if the CL is ready for review even if there are no problems,
+		// and possibly reminder comments followed by ultimately automatically
+		// abandoning the CL if the author never replies.
+		change, err := rules.ParseCommitMessage(repo.GetName(), cmsg)
+		if err != nil {
+			return fmt.Errorf("failed to parse commit message for %s: %v", prShortLink(pr), err)
+		}
+		problems := rules.Check(change)
+		if len(problems) > 0 {
+			summary := rules.FormatResults(problems)
+			// If needed, summary contains advice for how to edit the commit message.
+			msg := fmt.Sprintf("Hello %v, I've spotted some possible problems.\n\n"+
+				"These findings are based on simple heuristics. If a finding appears wrong, briefly reply here saying so. "+
+				"Otherwise, please address any problems and update the GitHub PR. "+
+				"When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://github.com/golang/go/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above.\n\n"+
+				"%s\n\n"+
+				"(In general for Gerrit code reviews, the CL author is expected to close out each piece of feedback by "+
+				"marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. "+
+				"See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)",
+				author, summary)
+
+			gcl, err := b.gerritChangeForPR(pr)
+			if err != nil {
+				return fmt.Errorf("could not look up CL after creation for %s: %v", prShortLink(pr), err)
+			}
+			unresolved := true
+			ri := gerrit.ReviewInput{
+				Comments: map[string][]gerrit.CommentInput{
+					"/PATCHSET_LEVEL": {{Message: msg, Unresolved: &unresolved}},
+				},
+			}
+			// TODO: instead of gcl.ID, we might need to do something similar to changeTriple in cmd/coordinator.
+			err = b.gerritClient.SetReview(ctx, gcl.ChangeID, gcl.ID, ri)
+			if err != nil {
+				return fmt.Errorf("could not add findings comment to CL for %s: %v", prShortLink(pr), err)
+			}
+		}
+	}
+
+	return nil
 }
 
 var changeIdentRE = regexp.MustCompile(`(?m)^Change-Id: (I[0-9a-fA-F]{40})\n?`)