Require a single approval on unprotected target branches

.github/workflows/review-trigger.yml

@@ -9,6 +9,8 @@ on:
       - review_requested
       - review_request_removed
       - ready_for_review
+      # Fires when the base branch changes, which changes what the rules require
+      - edited
   pull_request_review:
 
 jobs:

README.md

@@ -52,6 +52,8 @@ on:
       - review_requested
       - review_request_removed
       - ready_for_review
+      # Fires when the base branch changes, which changes what the rules require
+      - edited
   pull_request_review:
 
 jobs:
@@ -220,13 +222,17 @@ interface Report {
   }[];
 }
 ```
+
+> [!NOTE]
+> When the requirements were relaxed because the Pull Request targets an unprotected branch, `report` contains one single entry named `Single approval` with a `type` of `single-approval`. See [singleApprovalOnUnprotectedBranches](#singleapprovalonunprotectedbranches).
+
 ## Rule configuration file
 This is the file where all the available rules are written. 
 
 **This file is only read from the main branch.** So if you modify the file, the changes won’t happen until it is merged into the main branch. 
 This is done to stop users from modifying the rules in their PRs.
 
-It contains an object called `rules` which has an array of rules. Every rule has a same base structure. There is also a second optional field called `preventReviewRequests`.
+It contains an object called `rules` which has an array of rules. Every rule has a same base structure. There are also two optional fields called `preventReviewRequests` and `singleApprovalOnUnprotectedBranches`.
 ```yaml
 rules:
   - name: Rule name
@@ -244,6 +250,8 @@ preventReviewRequests:
   teams:
     - team-a
     - team-b
+
+singleApprovalOnUnprotectedBranches: true
 ```
 
 #### Rules fields
@@ -267,6 +275,42 @@ This is a special field that applies to all the rules.
 
 This field is **optional** and currently not used. Pending on https://github.com/paritytech/review-bot/issues/53
 
+#### singleApprovalOnUnprotectedBranches
+This is a special field that applies to all the rules. It is **optional** and it **defaults to `true`**.
+
+When a Pull Request targets a branch that is *not* gated by reviews, all the rules that match the Pull Request are merged into a single requirement of **one approval**. A single approval from anyone who qualifies under *any* of the matching rules turns the check green.
+
+A branch is considered gated when any of the following is true:
+- It is the repository’s default branch (usually `main` or `master`).
+- It has [classic branch protection](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches) enabled.
+- A [ruleset](https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/about-rulesets) with a `pull_request` or `required_status_checks` rule applies to it. Rulesets that only forbid things like deletions or force pushes don’t count, as they don’t gate a merge.
+
+If the protection of the branch can not be evaluated (for example, because the token lacks permissions), the branch is treated as gated so the review requirements are never lowered by accident.
+
+A relaxed result is published as a **separate status check named `review-bot-relaxed`**, never as `review-bot`. Check runs belong to a commit rather than to a Pull Request, so two Pull Requests that share a head commit share their check run. Keeping the two names apart means a relaxed result can never satisfy the `review-bot` check that a protected branch requires, whether the Pull Request is later retargeted at a protected branch or another Pull Request is opened from the same commit. Only require `review-bot` in your branch protection, never `review-bot-relaxed`.
+
+Retargeting a Pull Request at a protected branch therefore leaves it without the required `review-bot` check until Review Bot evaluates it again. Make sure your trigger workflow includes the `edited` event, which is the one GitHub fires when the base branch changes.
+
+The reasoning is that a Pull Request targeting a long lived feature branch or a stacked branch still has to pass every rule in full once those changes target a protected branch, so collecting the whole set of approvals twice only slows the work down. A single approval is still required so that no change lands completely unreviewed.
+
+Set it to `false` to enforce every rule on every branch:
+
+```yaml
+rules:
+  - name: Default review
+    condition:
+      include:
+        - '.*'
+    type: basic
+    teams:
+      - team-example
+
+singleApprovalOnUnprotectedBranches: false
+```
+
+> [!NOTE]
+> The `read` permission on `contents` is enough to evaluate branch protection and rulesets, so no extra token scope is needed.
+
 
 ### Types
 Every type has a *slightly* different configuration and works for different scenarios, so let’s analyze all of them.

action.yml

@@ -32,4 +32,4 @@ outputs:
 
 runs:
   using: 'docker'
-  image: 'docker://ghcr.io/paritytech/review-bot/action:2.7.2'
+  image: 'docker://ghcr.io/paritytech/review-bot/action:2.8.0'

package.json

@@ -1,6 +1,6 @@
 {
   "name": "review-bot",
-  "version": "2.7.2",
+  "version": "2.8.0",
   "description": "Have custom review rules for PRs with auto assignment",
   "main": "src/index.ts",
   "scripts": {

src/failures/index.ts

@@ -1,3 +1,4 @@
 export * from "./types";
 export * from "./commonRules";
 export * from "./fellowsRules";
+export * from "./singleApprovalRule";

src/failures/singleApprovalRule.ts

@@ -0,0 +1,62 @@
+import { summary } from "@actions/core";
+
+import { toHandle } from "../util";
+import { RequiredReviewersData, ReviewFailure, RuleFailedSummary } from "./types";
+
+/** Failure of the single requirement that replaces every rule when a pull request targets a
+ * branch which is neither the repository's default branch nor a protected branch.
+ *
+ * All the rules that matched the pull request are merged into this one, so a single approval
+ * from any of the qualifying users fulfills every one of them.
+ */
+export class SingleApprovalFailure extends ReviewFailure {
+  public readonly usersToRequest: string[];
+  public readonly teamsToRequest: string[];
+
+  constructor(
+    report: Omit<RuleFailedSummary, "missingReviews"> & RequiredReviewersData,
+    /** The branch that the pull request wants to merge into */
+    public readonly targetBranch: string,
+    /** Names of the rules that were merged into this requirement */
+    public readonly mergedRules: string[],
+  ) {
+    super({ ...report, missingReviews: 1 });
+    this.usersToRequest = report.usersToRequest ?? [];
+    this.teamsToRequest = report.teamsToRequest ?? [];
+  }
+
+  ruleExplanation(): string {
+    return (
+      `This pull request targets \`${this.targetBranch}\`, which is neither the repository's default branch ` +
+      "nor a protected branch, so all the rules that matched were merged into a single requirement of one approval.\n\n" +
+      `The merged rules are: ${this.mergedRules.map((rule) => `\`${rule}\``).join(", ")}. ` +
+      "Every one of them is enforced in full once the changes target a protected branch."
+    );
+  }
+
+  generateSummary(): typeof summary {
+    let text = super.generateSummary();
+
+    if (this.usersToRequest.length > 0) {
+      text = text.addHeading("Missing users", 3).addList(this.usersToRequest);
+    }
+    if (this.teamsToRequest.length > 0) {
+      text = text.addHeading("Missing reviews from teams", 3).addList(this.teamsToRequest);
+    }
+
+    if (this.missingUsers.length > 0) {
+      text = text.addDetails(
+        "GitHub users whose approval counts",
+        `One approval from any of these GitHub users fulfills this requirement:\n\n - ${this.missingUsers
+          .map(toHandle)
+          .join("\n - ")}`,
+      );
+    }
+
+    return text;
+  }
+
+  getRequestLogins(): { users: string[]; teams: string[] } {
+    return { users: this.usersToRequest, teams: this.teamsToRequest };
+  }
+}

src/failures/types.ts

@@ -12,8 +12,13 @@ export type RuleFailedReport = {
   countingReviews: string[];
 };
 
+/** Identifies what produced a failure.
+ * Aside from the rules of the configuration file, review-bot generates requirements of its own.
+ */
+export type FailureType = RuleTypes | "single-approval";
+
 export type RuleFailedSummary = {
-  type: RuleTypes;
+  type: FailureType;
   name: string;
 } & RuleFailedReport;
 
@@ -29,7 +34,7 @@ export type RequiredReviewersData = {
  */
 export abstract class ReviewFailure {
   public readonly name: string;
-  public readonly type: RuleTypes;
+  public readonly type: FailureType;
   /** The amount of missing reviews */
   public readonly missingReviews: number;
 
@@ -47,7 +52,7 @@ export abstract class ReviewFailure {
     this.missingUsers = ruleInfo.missingUsers;
   }
 
-  ruleExplanation(type: RuleTypes): string {
+  ruleExplanation(type: FailureType): string {
     switch (type) {
       case RuleTypes.Basic:
         return "Rule 'Basic' requires a given amount of reviews from users/teams";

src/github/check.ts

@@ -3,6 +3,19 @@ import { PullRequest } from "@octokit/webhooks-types";
 
 import { ActionLogger, CheckData, GitHubClient } from "./types";
 
+/** Name of the check run that protected branches require to pass */
+export const CHECK_NAME = "review-bot";
+
+/** Name of the check run used when the requirements were relaxed to a single approval.
+ *
+ * Check runs are keyed by commit, not by pull request, so two pull requests that share a head
+ * commit share their check run. A relaxed result must never land on {@link CHECK_NAME}: it would
+ * satisfy the status check that a protected branch requires, either by being inherited when the
+ * pull request is retargeted or by overwriting the strict result of another pull request on the
+ * same commit.
+ */
+export const RELAXED_CHECK_NAME = "review-bot-relaxed";
+
 /** GitHub client with access to Checks:Write
  * Ideally, a GitHub action.
  * This is the solution to the https://github.com/paritytech/review-bot/issues/54
@@ -24,15 +37,18 @@ export class GitHubChecksApi {
    * {@link https://docs.github.com/en/rest/checks/runs?apiVersion=2022-11-28}
    * @param checkResult a CheckData object with the final conclussion of action and the output text
    * {@link CheckData}
+   * @param relaxed whether the requirements were relaxed to a single approval. Such a result is
+   * published under {@link RELAXED_CHECK_NAME} so it can never satisfy a protected branch.
    */
-  async generateCheckRun(checkResult: CheckData): Promise<void> {
+  async generateCheckRun(checkResult: CheckData, relaxed: boolean = false): Promise<void> {
+    const name = relaxed ? RELAXED_CHECK_NAME : CHECK_NAME;
     const checkData = {
       ...checkResult,
       owner: this.repoInfo.owner,
       repo: this.repoInfo.repo,
-      external_id: "review-bot",
+      external_id: name,
       head_sha: this.pr.head.sha,
-      name: "review-bot",
+      name,
       details_url: this.detailsUrl,
     };
 

src/github/pullRequest.ts

@@ -4,6 +4,12 @@ import { Reviewers } from "../rules/types";
 import { caseInsensitiveEqual } from "../util";
 import { ActionLogger, GitHubClient } from "./types";
 
+/** Ruleset rules that gate a merge, and so make a branch one that reviews must protect.
+ * Rules like forbidding deletions or force pushes are left out: rulesets commonly apply those to
+ * every branch of a repository, which would make every branch look protected.
+ */
+const GATING_RULES = ["pull_request", "required_status_checks"];
+
 /** API class that uses the default token to access the data from the pull request and the repository
  * If we are using the assign reviewers features with teams, it requires a GitHub app
  * (Action token doesn't have permission to assign teams)
@@ -24,6 +30,8 @@ export class PullRequestApi {
   private filesChanged: string[] = [];
   /** Cache for the list of logins that have approved the PR */
   private usersThatApprovedThePr: string[] | null = null;
+  /** Cache for the evaluation of the protection of the branch that the PR targets */
+  private targetBranchProtected: boolean | null = null;
 
   async getConfigFile(configFilePath: string): Promise<string> {
     this.logger.info(`Fetching config file in ${configFilePath}`);
@@ -138,4 +146,57 @@ export class PullRequestApi {
   getAuthor(): string {
     return this.pr.user.login;
   }
+
+  /** Returns the name of the branch that the PR wants to merge into */
+  getTargetBranch(): string {
+    return this.pr.base.ref;
+  }
+
+  /** Evaluates if the branch that the PR targets is one that needs to be gated by reviews.
+   *
+   * A branch qualifies if it is the repository's default branch, if it has classic branch
+   * protection enabled, or if a ruleset that gates pull requests applies to it.
+   *
+   * If the protection can not be evaluated we consider the branch protected, as failing to
+   * reach the API should never lower the review requirements of a PR.
+   */
+  async isTargetBranchProtected(): Promise<boolean> {
+    if (this.targetBranchProtected !== null) {
+      return this.targetBranchProtected;
+    }
+
+    const branch = this.getTargetBranch();
+
+    if (branch === this.pr.base.repo.default_branch) {
+      this.logger.info(`Branch '${branch}' is the repository's default branch.`);
+      this.targetBranchProtected = true;
+      return this.targetBranchProtected;
+    }
+
+    try {
+      const { data: branchData } = await this.api.rest.repos.getBranch({ ...this.repoInfo, branch });
+      if (branchData.protected) {
+        this.logger.info(`Branch '${branch}' has branch protection enabled.`);
+        this.targetBranchProtected = true;
+        return this.targetBranchProtected;
+      }
+
+      const { data: rules } = await this.api.rest.repos.getBranchRules({ ...this.repoInfo, branch });
+      this.logger.debug(`Rules applying to '${branch}': ${JSON.stringify(rules)}`);
+      // Rulesets often target every branch with rules that have nothing to do with reviews,
+      // like forbidding deletions, so only the rules that gate a merge count here.
+      this.targetBranchProtected = rules.some((rule) => GATING_RULES.indexOf(rule.type) > -1);
+      this.logger.info(
+        this.targetBranchProtected
+          ? `Branch '${branch}' is targeted by a ruleset that gates merges.`
+          : `Branch '${branch}' is neither the default branch nor a protected branch.`,
+      );
+    } catch (error) {
+      this.logger.warn(`Failed to evaluate the protection of branch '${branch}'. Considering it protected.`);
+      this.logger.warn(error as Error);
+      this.targetBranchProtected = true;
+    }
+
+    return this.targetBranchProtected;
+  }
 }

src/rules/types.ts

@@ -63,4 +63,13 @@ export interface ConfigurationFile {
     users?: string[];
   };
   score?: FellowsScore;
+  /** Pull requests that target a branch which is neither the repository's default branch nor a
+   * protected branch are merged into a single requirement of one approval.
+   *
+   * Such branches do not gate anything, so requiring the full set of reviews only slows down
+   * work that still has to pass every rule once it targets a protected branch.
+   * Set it to `false` to enforce every rule on every branch.
+   * @default true
+   */
+  singleApprovalOnUnprotectedBranches?: boolean;
 }

src/rules/validator.ts

@@ -51,6 +51,7 @@ export const generalSchema = Joi.object<ConfigurationFile>().keys({
   rules: Joi.array<ConfigurationFile["rules"]>().items(ruleSchema).unique("name").required(),
   preventReviewRequests: Joi.object().keys(reviewersObj).optional().or("users", "teams"),
   score: fellowScoreSchema,
+  singleApprovalOnUnprotectedBranches: Joi.boolean().default(true),
 });
 
 /** Basic rule schema